[P4Testgen] Introduce the option to produce lowercase hex for the formatting library. #4340
Conversation
| bool useSep = false; | ||
| bool pad = true; | ||
| bool usePrefix = true; | ||
| bool writeUpperCaseHex = true; |
There was a problem hiding this comment.
Nit: any reason why we say "write" here, but "use" for some of the other options? If not, shall we be consistent?
| @@ -58,7 +58,7 @@ const Expression *Expression::evaluate(const Model &model, bool doComplete) cons | |||
|
|
|||
| void Expression::print(std::ostream &os) const { | |||
| Generic::print(os); | |||
| os << ": " << formatHexExpr(value, true); | |||
| os << ": " << formatHexExpr(value, {true}); | |||
There was a problem hiding this comment.
{true}: This isn't great for code readability, use designated initializers instead?
{.useSep = true}
It is technically a C++20 feature, so may not be supported?
Ditto a couple of more times, throughout.
There was a problem hiding this comment.
Yes, the compiler still uses C++17. :/ But I do not think there is any reason why we can't move to C++20. Would unlock some nice things. Let me start a discussion on this.
There was a problem hiding this comment.
My main concern is if this requires bumping the minimal required version of GCC. If so, that could be problematic for downstream tools, or even for users of p4c directly. Not much of C++20 is supported in GCC 9. Designated initializers are, because they were GNU extension for a long time... But this should be probably discussed elsewhere.
…ormatting library. Add options structure.
fruffy-g
force-pushed
the
fruffy/format_hex_lowercase
branch
from
ca7b83c
to
a87d861
Compare
Also add an options structure to the library