Generating constexpr for constants when possible #282
Conversation
|
Automated message from Dropbox CLA bot @itsff, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here. |
|
Signed. |
|
Automated message from Dropbox CLA bot @itsff, thanks for signing the CLA! |
|
Nice idea, however the tests are only half-way there. To make sure everything links properly the header-only constants need to be ODR-used in at least two translation units. Furthermore I wouldn't count on |
|
There was talk back in March in the djinni Slack channel about moving the requirement to C++14. Should this again be considered now? |
|
|
There was a problem hiding this comment.
Looks great in concept. I've made a few suggestions inline regarding style and clarity.
I'm torn on the question of requiring optional to support constexpr. Both std::experimental::optional (the pre-C++17 version) and std::optional (in C++17) were specified to have constexpr constructors, and I'd be keen to benefit from that in our own code. However boost::optional doesn't, and I'm pretty sure there are users out there making use of that. This might be something which requires a new command-line flag so that users can specify whether their optional implementation supports constexpr, along with specifying its class name and header file.
Adding a test in the test-suite to ensure that ODR isn't violated is a good suggestion too.
| case _ => { | ||
| needsCppGen = true; | ||
| isConstExpr = false; | ||
| ";" |
There was a problem hiding this comment.
I'd suggest making constValue empty here, and applying the semi-colon in the final statement below. The semicolon isn't really part of the const value.
| w.w(s"${marshal.fieldType(c.ty)} const $selfName::${idCpp.const(c.ident)} = ") | ||
| writeCppConst(w, c.ty, c.value) | ||
| w.wl(";") | ||
| val isConstExpr = c.value match { |
There was a problem hiding this comment.
It would be nice to unify the cases for this match with the ones above in the header file. One option would be to make a helper function which returns the constValue used in the header file, then have isConstExpr here simply test whether that value is non-empty.
| for (c <- consts) { | ||
| var isConstExpr = true | ||
| val constValue = c.value match { |
There was a problem hiding this comment.
It might be clearer to match on c.ty instead. I'm not set on that, since this does make the code simpler, but it took me a while to figure out that this distinction is the reason that optionals are being treated as constexpr-eligible. It's not an explicit case statement anywhere, but simply implicit in the fact that the constant value for an optional is of type T.
I've noticed that the constants in generated C++ code are always initialized in the cpp file, even for basic types like bool and numerics.
In this patch I've modified the C++ code generator to determine if a value can be initialized in the header and, if so, place it there as a constexpr. This only happens for bools, numerics, enums and optionals thereof. If any other kind of constant is present, its value will get set in the implementation file as before.
Please refer to the "generated-src" from the test suite which is part of this commit.