Implement Constexpr Strong Typedef #861
Conversation
|
Review changes with SemanticDiff. |
jwellbelove
changed the base branch from
development
to
pull-request/#861-Implement-Constexpr-Strong-Typedef
jwellbelove
merged commit 48c496c
into
ETLCPP:pull-request/#861-Implement-Constexpr-Strong-Typedef
| { | ||
| value = rhs.value; | ||
| return *this; | ||
| } | ||
|
|
||
| //********************************************************************* | ||
| TValue& get() | ||
| ETL_CONSTEXPR TValue& get() |
There was a problem hiding this comment.
@jwellbelove I believe I may have mistaken this part. I think for C++11 users this will make the TValue reference be const? In C++14 they loosened the restrictions on what constexpr meant, so methods didn't have to be necessarily constexpr to have the keyword for it. I can change this quickly if you suspect this is the case.
There was a problem hiding this comment.
Yes, it is normal to not have constexpr on both the const and non-const functions.
I'm pretty sure you don't need the #if ETL_USING_CPP14, unless I'm missing something.
I'm also sure that the operators can also be constexpr.
There was a problem hiding this comment.
What would you like? I thought for CPP11 constexpr was strict? I can leave the constexpr off the get methods. As far as operators, I left the assignment operators (*=, +=, -=, ++, --, etc) not constexpr because there wasn't a way for me to test them. In C++14 it would have been okay to make them constexpr because of the more lax rules. But I'm pretty sure that would have failed the C++11 actions. I think i'll just leave the gets as non-constexpr and open a new PR
There was a problem hiding this comment.
I found I get a compile error if I've set both const and non-const versions of a member function constexpr.
The operators can be tested by making a unit test that is only enabled for C++14 and above.
The CI that I run locally, and on Github Actions, test with C++11, C++14, C++17 & C++20
There was a problem hiding this comment.
I will update the tests for C++14 and above, thanks.
There was a problem hiding this comment.
I'm not quite sure how they can be constexpr without specifying #if ETL_USING_CPP14, can you clarify? It's not making sense how the C++11 builds would pass since all the operator *= += -= tests will be non-constexpr. As you pointed out, I would have to create separate tests for C++14 and above - which is "ETL_USING_CPP14" if I'm not mistaken. Why would the tests need differentiated but not the implementation?
There was a problem hiding this comment.
Part of me thinks the assignment operators shouldn't be marked constexpr regardless because you can't manipulate a constexpr variable. So I almost feel like the PR should stay the way it is (I removed the constexpr keywords from the get and assignment operators).
There was a problem hiding this comment.
You can able to use constexpr operators inside a constexpr function that returns a constexpr object.
Example
https://godbolt.org/z/rPqW17xPx
There is more than one constexpr macro.
ETL_CONSTEXPR or ETL_CONSTEXPR11 Defined as constexpr for C++11 and above
ETL_CONSTEXPR14 Defined as constexpr for C++14 and above
ETL_CONSTEXPR17 Defined as constexpr for C++17 and above
ETL_CONSTEXPR20 Defined as constexpr for C++20 and above
You use the one that is appropriate for the code.
There was a problem hiding this comment.
Thank you, much appreciated! I will get right on that
There was a problem hiding this comment.
I'm still getting build errors. I'm kind of stumped. The lambdas were nice to keep the typedefs local to the test functions. If I move the lambdas out to constexpr free functions like your godbolt-linked example, then the typedef will need to exist at a scope outside of the tests (which will conflict with the tests). Unless you're okay with that or have another suggestion
…def' into feature/838-strong-typedef-constexpr
For #838 and #844, added the ability to make constexpr strong types.