-
Notifications
You must be signed in to change notification settings - Fork 191
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
Test complex elliptic DG operator #6269
Conversation
df706b8
to
2536a26
Compare
cda99b9
to
ce983e1
Compare
@wthrowe would you be able to review this one as well? (continuing the quest of DG with complex numbers) |
const double constant; | ||
// NOLINTNEXTLINE(cppcoreguidelines-avoid-const-or-ref-data-members) | ||
const double complex_phase; |
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.
The clang-tidy warnings look right, at least for these last two. Why should they be ignored? Similar comments for the other file.
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 don't want functions in this class be able to modify this value. It's the user-defined value from options that gets passed down to this struct. I don't understand why clang-tidy want's me to remove the const
when really I want it to be const.
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.
The documentation links to the core guidelines rule and some extra reading. I don't agree with everything in there, but that section seems pretty good. The short answer for your class is that the functions already can't modify the value because they are const member functions.
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.
Ok makes sense, I removed the const
ce983e1
to
2b8ee9c
Compare
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.
Proposed changes
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments