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

Fix minor integer type mismatches #3133

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

heplesser
Copy link
Contributor

This PR fixes some warnings by GCC 12.3 about integer type mismatches.

Note that we have a bit of a mess between int (e.g., MPIManager::num_processes_) and size_t (e.g., return type of MPIManager::get_num_processes()), but cleaning that up is for another day.

@heplesser heplesser added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Mar 5, 2024
Copy link
Contributor

@otcathatsya otcathatsya left a comment

Choose a reason for hiding this comment

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

LGTM! As a related question, would we be interested in applying type/tidiness suggestions (e.g. "can be made const", "can be made static") to all files? Some IDEs conveniently provide the feature to analyze all files in a scope and they should be safe.

@JanVogelsang
Copy link
Contributor

LGTM! As a related question, would we be interested in applying type/tidiness suggestions (e.g. "can be made const", "can be made static") to all files? Some IDEs conveniently provide the feature to analyze all files in a scope and they should be safe.

While this is a good idea, it is preferable to find some tools which can be invoked from the CI pipeline so that the code-refactoring would be a continuous process and also applies to new code rather than just a one-time thing for the current codebase.
Most IDEs just use clang-format/clang-tidy or similar tools under the hood, which we could use in our pipelines as well.

@JanVogelsang
Copy link
Contributor

but cleaning that up is for another day

Could you open an issue in which you mention the mess? It would be great if we would collect a list of potential refactorings that one could collectively do at some point, or ideally use these as a test case for automated refactoring tools (e.g. clang-tidy).

@otcathatsya
Copy link
Contributor

LGTM! As a related question, would we be interested in applying type/tidiness suggestions (e.g. "can be made const", "can be made static") to all files? Some IDEs conveniently provide the feature to analyze all files in a scope and they should be safe.

While this is a good idea, it is preferable to find some tools which can be invoked from the CI pipeline so that the code-refactoring would be a continuous process and also applies to new code rather than just a one-time thing for the current codebase. Most IDEs just use clang-format/clang-tidy or similar tools under the hood, which we could use in our pipelines as well.

I definitely agree that we could use that for CI, though might not have to be mutually exclusive. A lot of the IDE & static analysis tools go beyond clang-tidy, though it'd probably be best to have a session and go over some of that output together at some point since not every refactor is safe but may very well cut down build or even runtime (missing constexprs, unused imports, redundant checks etc)

@heplesser heplesser changed the title Fix warnings issued by gcc 12.3 Fix minor integer type mismatches Mar 6, 2024
@heplesser heplesser merged commit f11a1fa into nest:master Mar 6, 2024
24 checks passed
@heplesser heplesser deleted the fix-gcc12-warnings branch April 24, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants