-
Notifications
You must be signed in to change notification settings - Fork 442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[P4Testgen] Introduce a configuration structure for the test back ends. #4372
Conversation
c14bb75
to
4aa0729
Compare
f749472
to
ac8f288
Compare
4aa0729
to
709a48f
Compare
ef11adf
to
107d409
Compare
709a48f
to
708dfa1
Compare
3edd94b
to
6d78adc
Compare
38da2dc
to
d2a5bb4
Compare
d2a5bb4
to
5ac93fd
Compare
auto optBasePath = getTestBackendConfiguration().fileBasePath; | ||
BUG_CHECK(optBasePath.has_value(), "Base path is not set."); | ||
auto incrementedbasePath = optBasePath.value(); | ||
incrementedbasePath.concat("_" + std::to_string(testId)); | ||
incrementedbasePath.replace_extension(".txtpb"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to lift this into a lib function (of the config?), parametrized by the test id and extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an annoyingly common pattern with std::optional
and std::expected
. Makes you wish C++ had a ?
operator.
I have been thinking about introducing https://github.com/TartanLlama/optional at some point, since it fills the gap std::optional
left with optional references and unpacking.
Or simply use some custom macro like this
#define ASSIGN_OR_RETURN_WITH_MESSAGE_IMPL(temporary, targetVariable, inputFunction, returnValue, \
errorFunction) \
auto(temporary) = inputFunction; \
if (!(temporary)) { \
errorFunction; \
return returnValue; \
} \
targetVariable = *(temporary); // NOLINT
#define ASSIGN_OR_RETURN_WITH_MESSAGE(targetVariable, inputFunction, returnValue, errorFunction) \
ASSIGN_OR_RETURN_WITH_MESSAGE_IMPL(STATUS_MACROS_CONCAT_NAME(temporary, __LINE__), \
targetVariable, inputFunction, returnValue, errorFunction)
There is also https://github.com/abseil/abseil-cpp/blob/master/absl/status/statusor.h.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created an issue to track this #4403.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the general case is interesting too, but here I meant the function to derive test file name from configuration, which is almost exactly the same in most testgen backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are more patterns that are similar in the test back ends. Any back end that writes to files and does not use a preamble (PTF) has a similar pattern. I could move the entire sequence into a helper.
However, I would like to clean up the error handling first before introducing a new utility function.
5ac93fd
to
81974aa
Compare
81974aa
to
fd7c6d7
Compare
fd7c6d7
to
a50b982
Compare
This pull request adds the
TestBackendConfiguration
object to P4Testgen. This object contains configuration options which are consumed by the test back ends. Most of the changes involve refactoring.Currently, the object has the testname, seed, the test path and the amount of tests to generate as inputs.