[P4Testgen] Extend the CompilerTarget runProgram function with data structures which can pass on more information. #4323
Conversation
fruffy
changed the title
fruffy
force-pushed
the
fruffy/compiler_info_1
branch
from
e437009 to
be3c6bd
Compare
There was a problem hiding this comment.
It would be really good to have more info about how this would be used and what would be the next steps. At this point, this just introduces a data structure that is not used in the end. I presume #4324 is the next step -- I'll have a look at that too as I can't really evaluate this one in isolation.
| /// | ||
| /// @returns std::nullopt if an error occurs during compilation. | ||
| static std::optional<const IR::P4Program *> runCompiler(); | ||
| static std::optional<const CompilerResult *> runCompiler(); |
There was a problem hiding this comment.
Is there any reason to return optional of a pointer? I don't see std::nullopt meaning something different than optional with nullptr (and if it did it would be quite confusing I think). I suggest using std::optional<CompilerResult> as the struct will likely mainly contain pointers/references and it will not be copied much around so this will matter even if it become larger.
There was a problem hiding this comment.
The pointer is necessary here because CompilerResult will be subclassed by the target back ends, e.g., BMv2CompilerResult etc. It could also be a reference, but that would require a reference_wrapper.
I much prefer using std::optional over nullptr these days because it makes it very explicit that an error has happened. Going forward it will also be easier to replace this with std::expect or absl:StatusOr.
There was a problem hiding this comment.
hmm, I did not realize that CompilerResult will be subclassed, that does make a lot of sense. I don't like that std::optional<pointer> solution much because it has the ugly state where the optional contains nullptr. You are right that optional is more explicit about having to resolve the error though :-/ (and std::optional<std::reference_wrapper<X>> looks ugly and has the additional disadvantage that auto x = opt.value() behaves weirdly.
There was a problem hiding this comment.
I'm starting to think that std::optional<std::reference_wrapper<const CompilerResult>> is the right option here if you want the added safety of optional. This is a bit uglier to use (the type is long, and one needs to explicitly call .get() in many cases). But it is unlikely the auto problem I mentioned above will cause problems exactly because one has to call get() to get the actual instance. And having std::optional<const CompilerResult *>(nullptr) would be really confusing. Too bad std does not allow std::optional<const CompilerResult &> :-/.
The idea is that you can pass any information from the compiler mid end downstream to any P4Tools module. This refactoring also ensures that we really only need to maintain one data structure for all this information, the |
fruffy
force-pushed
the
fruffy/compiler_info_1
branch
2 times, most recently
from
aa9b438 to
e9a24db
Compare
| @@ -31,7 +31,7 @@ std::optional<const P4ToolsTestCase> P4ToolsTestCase::create( | |||
| if (!compilerResults.has_value()) { | |||
| return std::nullopt; | |||
| } | |||
| return P4ToolsTestCase{compilerResults.value()->getProgram()}; | |||
| return P4ToolsTestCase(compilerResults.value().get().getProgram()); | |||
There was a problem hiding this comment.
Since you have already checked for nullopt, you can use compilerResults->get() if you want (operator-> and operator* are unchecked, unlike value()).
…tures which can pass on more information.
fruffy
force-pushed
the
fruffy/compiler_info_1
branch
from
e9a24db to
227ff4d
Compare
Major rewrite of the ProgramInfo and CompilerTarget infrastructure. The basic idea is to compute all necessary static analysis information in the compilation phase, then pass this information forward to ProgramInfo.
For this, we introduce a CompilerResult object. This object is produced in the compilation phase. Any P4Tools module can add arbitrary information to this class. The results object is then passed to ProgramInfo.
This PR is part 1. It introduces the
CompilerResultdata structure, which records information produces by the compiler which runs before P4Testgen is invoked. This includes the P4 program after the mid end for now.