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

Suppress modernize-use-integer-sign-comparison #4558

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Conversation

nlohmann
Copy link
Owner

The modernize-use-integer-sign-comparison was added to Clang-Tidy and is only applicable to C++20 code. Therefore, we suppress this check.

@nlohmann nlohmann added this to the Release 3.11.4 milestone Dec 18, 2024
@nlohmann nlohmann self-assigned this Dec 18, 2024
@github-actions github-actions bot added the S label Dec 18, 2024
@gregmarr
Copy link
Contributor

Is this showing a lot of hits? Should we create a helper function for pre-c++20 that makes those comparisons safer?

@nlohmann
Copy link
Owner Author

Is this showing a lot of hits? Should we create a helper function for pre-c++20 that makes those comparisons safer?

See https://github.com/nlohmann/json/actions/runs/12396405659/job/34604358473?pr=4557.

The first hit is this:

/__w/json/json/include/nlohmann/detail/input/binary_reader.hpp:1112:13: error: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison,-warnings-as-errors]
 1112 |         if (len != static_cast<std::size_t>(-1))
      |             ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~   
      |             std::cmp_not_equal( ,              )

len is of type std::size_t here. Not sure what to make out of it.

Yes, one approach could be to have our own implementation of cmp_equal, cmp_not_equal, cmp_less, cmp_greater, cmp_less_equal, and cmp_greater_equal and use it throughout. I would still like to merge this first, because all pipelines will fail now without suppressing this issue.

Are you OK with this and having an issue to fix this in the future?

@gregmarr
Copy link
Contributor

Yeah, since all the current usages are safe, and the compiler is apparently looking right through the cast that we're using to make it "safe", then I think it's fine to log it for the future. Maybe we can use std::numeric_limits<std::size_t>::max() instead of static_cast<size_t>(-1).

@nlohmann nlohmann merged commit 6cb099e into develop Dec 18, 2024
121 checks passed
@nlohmann nlohmann deleted the fix-clang-tidy branch December 18, 2024 16:44
@coveralls
Copy link

Coverage Status

coverage: 99.634%. remained the same
when pulling 0f7836f on fix-clang-tidy
into 094bd26 on develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants