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.

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

[Trivial] Fix type used for array init #1170

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

pgrete
Copy link
Collaborator

@pgrete pgrete commented Sep 9, 2024

PR Summary

Clang just warned me about this. Apparently this was introduced because of clang (2fcd219).
Let's see what CI says (haven't tested locally)

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@Yurlungur
Copy link
Collaborator

Good catch. Surprised we didn't see that before...

@pgrete
Copy link
Collaborator Author

pgrete commented Sep 10, 2024

/Users/runner/work/parthenon/parthenon/src/outputs/restart_hdf5.cpp:217:17: error: constexpr variable 'VNDIM' must be initialized by a constant expression
  constexpr int VNDIM = info.VNDIM;
                ^       ~~~~~~~~~~
/Users/runner/work/parthenon/parthenon/src/outputs/restart_hdf5.cpp:217:25: note: function parameter 'info' with unknown value cannot be used in a constant expression
  constexpr int VNDIM = info.VNDIM;
                        ^
/Users/runner/work/parthenon/parthenon/src/outputs/restart_hdf5.cpp:209:64: note: declared here
                                   const OutputUtils::VarInfo &info,
                                                               ^

I guess that's why the constexpr was removed in first place. Given the danger of a rabbit whole I won't be able to look at this before leaving. Will look at this again in two weeks unless someone else figures out a quick fix.

@Yurlungur
Copy link
Collaborator

Pretty sure the following will work:

constexpr int VNDIM = OutputUtils::VarInfo::VNDIM;

will try it and push.

@Yurlungur Yurlungur self-assigned this Sep 10, 2024
@Yurlungur Yurlungur added the bug Something isn't working label Sep 10, 2024
@Yurlungur Yurlungur changed the title Fix type used for array init [Trivial] Fix type used for array init Sep 10, 2024
Copy link
Collaborator

@lroberts36 lroberts36 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -47,7 +47,7 @@ namespace OutputUtils {
// Helper struct containing some information about a variable
struct VarInfo {
public:
static constexpr int VNDIM = MAX_VARIABLE_DIMENSION;
static constexpr const int VNDIM = MAX_VARIABLE_DIMENSION;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the extra const here redundant given that it is already constexpr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants