Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Capability adds for ParmParse enum #4119

Merged
merged 7 commits into from
Sep 4, 2024
Merged

Capability adds for ParmParse enum #4119

merged 7 commits into from
Sep 4, 2024

Conversation

baperry2
Copy link
Contributor

Summary

  • The ParmParse functions for AMReX Enums added in AMREX_ENUM and ParmParse support for enum class #4069 did not carry the optional arguments found in other ParmParse functions. Those are added so that, for example, querying an enum has the same interface as querying other types.
  • These functions shouldn't modify the ParmParse object, so they are made const
  • The prefixedName function is made a public member instead of a protected member.

Additional background

The use case for these changes is erf-model/ERF#1772

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • include documentation in the code and/or rst files, if appropriate

@baperry2 baperry2 mentioned this pull request Aug 30, 2024
Merged
@baperry2
Copy link
Contributor Author

I got clang-tidy complaints about ParmParse add and addarr functions not being const - I don't think that should be related to what I changed. In any event, I tried making all those const, which seems to be fine other than breaking the pyamrex interface. @WeiqunZhang and @asalmgren, any thoughts on what to do with this issue? Otherwise, this PR should be ready for review.

By the way, I really like the added capability for parmparsing enums. I can already think of a ton of places where that can drastically simplify code in various apps.

@WeiqunZhang
Copy link
Member

Re: constness, one could argue that add and addarr are expected to change the logical state of ParmParse. So I think we should leave them as non-const functions. If clang-tidy complains, let's use NOLINT(readability-make-member-function-const).

Yes, we should add const to query and get.

https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-const

@baperry2
Copy link
Contributor Author

baperry2 commented Sep 3, 2024 •
edited
Loading

Agreed that add/addarr should intuitively not be const despite what clang-tidy thinks. I added some NOLINTs for those functions instead of making const.

WeiqunZhang
WeiqunZhang reviewed Sep 4, 2024
View reviewed changes
Src/Base/AMReX_ParmParse.H Outdated
if (exist) {
try {
ref = amrex::getEnum<T>(s);
} catch (...) {
amrex::Print() << "amrex::ParmParse::query (input name: "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to allow the users to turn off warning messages. So we either remove this, or put this inside if (amrex::Verbose() > xxx), or add the message to what is being thrown by amrex::getEnum.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@WeiqunZhang WeiqunZhang enabled auto-merge (squash) September 4, 2024 20:14
@WeiqunZhang WeiqunZhang merged commit c88b2b5 into AMReX-Codes:development Sep 4, 2024
59 checks passed
@baperry2 baperry2 deleted the parmparse-enum branch September 6, 2024 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WeiqunZhang WeiqunZhang WeiqunZhang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@baperry2 @WeiqunZhang