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
Use switch statements in ReferenceCell #14680
Conversation
We can enable constexpr and disable the check on older compilers.
Patches like ebd03d4 indicate that there's a measurable performance advantage to not using a sequence of if statements.
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.
Looks good to me!
|
||
Assert(false, ExcNotImplemented()); | ||
return 0; | ||
return numbers::invalid_unsigned_int; |
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.
Nice!
} | ||
case ReferenceCells::Triangle: | ||
{ | ||
static constexpr ndarray<unsigned int, 3, 2> subcells = { |
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.
Also nice catch!
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.
Interesting. I'm not sure it's a clear win, but it isn't a loss either, so I'm happy to go with it.
In some future C++ standard (I forgot whether it was C++17 or 20 or 23), we will be able to write
switch (*this)
as long as there is a constexpr
operator==
. But for the moment, switch (this->kind)
will do just fine.
It passes all tests, so let's go with it! |
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.
Sorry for being late. If I remember correctly, we had problems with the switches and ReferenceCell
at some time somewhere in the code so that we had to remove that. I don't remember this PR that was...
|
||
Assert(false, ExcNotImplemented()); | ||
return 0; | ||
return numbers::invalid_unsigned_int; |
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.
This was intentionally. See #14283.
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.
You're right - lets change that back.
The problems came from the constructor having an assertion in it - everything should work now that we guard that assertion with our |
See also #13922.
This works with GCC 5.4 - we'll see if it works with other older compilers/MSVC/etc. In particular, we can work around
DEAL_II_CXX14_CONSTEXPR_BUG
by only running the assertion if the compiler supports assertions in constexpr functions.