Skip to content

Fix -Wsign-compare compiler warning #151

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

Closed
wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Feb 4, 2025

Fixes bitcoin/bitcoin#30975 (comment):

... then found another issue:

/home/runner/work/_temp/src/ipc/libmultiprocess/src/mp/gen.cpp:633:26: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
  633 |     for (size_t i = 4; i < argc; ++i) {
      |                        ~ ^ ~~~~
1 error generated.

This is the only warning when all default Bitcoin Core flags are applied.

@hebasto hebasto changed the title Fix -Wsign compiler warning Fix -Wsign-compare compiler warning Feb 4, 2025
Comment on lines +633 to 634
for (size_t i = 4; i < static_cast<size_t>(argc); ++i) {
KJ_IF_MAYBE(dir, fs->getRoot().tryOpenSubdir(cwd.evalNative(argv[i]))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In commit "Fix -Wsign-compare compiler warning" (7452ddd)

Is switching size_t on this line to int enough to fix the problem? It would seem better to avoid this error by using the same type consistently and avoiding the need to cast.

Copy link
Collaborator

Choose a reason for hiding this comment

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

re: #151 (comment)

Went ahead and made this change in #152 but did not confirm it fixes the warning (since I wasn't seeing the warning). If not, can use the fix here instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can confirm that the commit from #152 fixed the warning.

ryanofsky added a commit that referenced this pull request Feb 5, 2025
0b8b7f9 refactor: Fix `-Wsign-compare` compiler warning (Hennadii Stepanov)
a02c079 clang-tidy: Suppress `performance-enum-size` check warning (Hennadii Stepanov)
593807a clang-tidy: Fix `readability-container-size-empty` check (Hennadii Stepanov)
68c1c6c clang-tidy: Fix `readability-avoid-return-with-void-value` check (Hennadii Stepanov)
4abaa98 clang-tidy: Fix `readability-avoid-nested-conditional-operator` check (Hennadii Stepanov)
01ef094 clang-tidy: Fix `performance-unnecessary-value-param` check (Hennadii Stepanov)
c665a43 clang-tidy: Fix `misc-use-internal-linkage` check (Hennadii Stepanov)
15c77e9 clang-tidy: Fix `misc-include-cleaner` check (Hennadii Stepanov)
848c902 clang-tidy: Fix `misc-const-correctness` check (Hennadii Stepanov)
4a2508c clang-tidy: Fix `modernize-type-traits` check (Hennadii Stepanov)
8170d3d clang-tidy: Suppress `bugprone-empty-catch` check warning (Hennadii Stepanov)
c068596 clang-tidy: Fix `bugprone-crtp-constructor-accessibility` check (Hennadii Stepanov)
00036c0 clang-tidy: Disable `performance-avoid-endl` check (Hennadii Stepanov)
359a615 clang-tidy: Disable `misc-use-anonymous-namespace` check (Hennadii Stepanov)

Pull request description:

  Most of these changes are not my own. This is combination of commits from #144 and #151 which fix a variety of compiler warnings.

  Several of the fixes are just suppressions, so followups can be done to improve code and fix problems more completely. More details can be found in #144.

Top commit has no ACKs.

Tree-SHA512: 581720d357dce36553f91fb4bf92c4ade2c228ce199b342c28637f2615b22aa6e5908c83a698320438e5d53309581ec64410f469cbb782843970caee3c21d62e
@hebasto hebasto closed this Feb 5, 2025
@hebasto hebasto deleted the 250204-wsign branch February 5, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants