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
Don't use implementation-reserved double underscores #15870
Conversation
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 would approve, but could you provide slightly more detail on why this became an issue? I agree though that it's not a good practice to use the reserved __ suffix, I have bad memories of issues arising from that.
@blaisb I read through the NOX implementation to see how the status check is combined and stumbled over these two problematic variable names. There was no concrete bug or the like arising from them, though. It must still be fixed since we may not use such implementation-reserved identifiers. Also, I greped through the code base but couldn't find another occurrence. clang-tidy's bugprone-reserved-identifier would check for this. |
/rebuild |
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.
umh
The C++ standard reserves identifiers starting with double underscores, a single underscore in global namespace, or underscore and capital letter: __reserved
, __Reserved
. (It also reserves literal suffixes with double underscores if I remember correctly).
It does not restrict trailing underscores in variable names.
But anyway, let's get rid of the trailing underscores. We (as in deal.II project) try to avoid those as well.
@tamiko fyi
(cited from the draft of C++ 20, emphasis mine) You may not use double underscores anywhere within an identifier. |
Oh, this changed in C++11. Modern times... |
How interesting -- I didn't know about that either. Would you be interested in writing a follow-up patch that adds the clang-tidy flag you mention? |
No description provided.