GCC added the -Wtype-limits warning group to -Wextra around
GCC 4.4 and the group has some very helpful extra warnings
like tautological comparison type limit warnings
(comparingan unsigned int to see if it's positive, etc).
Details
Details
- Reviewers
aaron.ballman dblaikie thesamesam - Group Reviewers
Restricted Project - Commits
- rG95668c0d97e6: [Clang] Add -Wtype-limits to -Wextra for GCC compatibility
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Heh, I literally started working on this on Friday afternoon. :-D
There's a failing CI case that needs to be addressed: Sema/tautological-constant-compare.c -- IIRC, fixing that should suffice for test coverage. This should also have a release note.
Also, I'm adding clang-vendors not because I think this is a potentially *breaking* change, but because it could be potentially disruptive due to adding to -Wextra when we've not really changed that much in recent history.
| clang/docs/ReleaseNotes.rst | ||
|---|---|---|
| 45–46 | I'd move this into bug fixes as I don't think this is enough of a potential disruption to warrant putting it here. If we get user feedback about surprises, we might move it back to here later. | |
| clang/test/Sema/tautological-constant-compare.c | ||
| 7–9 | I'd change this the other way; this way we demonstrate that -Wextra is what enabled -Wtype-limits. | |
| clang/test/Sema/tautological-constant-compare.c | ||
|---|---|---|
| 7–9 | Ooops, I'm sorry, I forgot about this edit. :-( Once you make that change on the RUN lines, you'll remove all the additional warnings you added. | |
Comment Actions
LGTM, thank you!
Btw, if you change:
Fix https://github.com/llvm/llvm-project/issues/58375
into:
Fixes #58375
in the commit message, it will automatically close the github issue and associate it with this commit.
Comment Actions
@aaron.ballman It works with the URL as well. The URL is preferred, because that way people don't have to reconstruct it when seeing the issue reference on phabricator, where it does not get auto-linked.
Comment Actions
This discovers a warning in https://reviews.llvm.org/rGa68d4b11465f5b3326be1dd820f59fac275b7581
I think its checking is valid if size_t is uint32_t (eg. -m32)
Could you guys teach me what the right fix for it? I don't know canonical fixes for it.
Comment Actions
Hey, looks like this broke the x86_64-linux sanitizer buildbot: https://lab.llvm.org/buildbot/#/builders/37/builds/19910
You can reproduce the bot using the instructions at https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild (substitute buildbot_fast.sh for buildbot_cmake.sh to get the right one, but I don't think using the official buildscript is necessary. This looks like a 2-stage compile is not currently -Wextra -Werror clean:
/b/sanitizer-x86_64-linux/build/llvm-project/llvm/lib/ProfileData/InstrProfReader.cpp:177:41: error: result of comparison 'uint64_t' (aka 'unsigned long') > 18446744073709551615 is always false [-Werror,-Wtautological-type-limit-compare]
if (uint64_t(Buffer->getBufferSize()) > std::numeric_limits<uint64_t>::max())
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/llvm-project/llvm/lib/ProfileData/InstrProfReader.cpp:227:41: error: result of comparison 'uint64_t' (aka 'unsigned long') > 18446744073709551615 is always false [-Werror,-Wtautological-type-limit-compare]
if (uint64_t(Buffer->getBufferSize()) > std::numeric_limits<uint64_t>::max())
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~I tried to whack-a-mole those two bugs (see my two commits on https://lab.llvm.org/buildbot/#/builders/37/builds/19926), but there's more latent bugs hanging around.
Probably worth reverting this and the warnings need to be fixed inside clang/llvm first, then we can make it the default.
Comment Actions
+1 we have some code like this too and I'm not sure what's the best way to rewrite it to pacify this warning.
Comment Actions
This might be pointing out an issue with our existing -Wtype-limits implementation, as the way I would correct that still gets diagnosed: https://godbolt.org/z/sY9PodKbf The only time the comparison is tautological is when size_t has less width than uint64_t, but the extra test for that scenario doesn't silence the diagnostic. Even when I try really hard: https://godbolt.org/z/PbWcTYaz1
Comment Actions
There are also cases that this clang type-limit check will warn when checking if a enum type is larger than 0. For example, we are seeing warnings on ICU source code (https://github.com/unicode-org/icu/blob/8d5a97ae0f49f6974372736ca67db15c37522f6f/icu4c/source/i18n/displayoptions.cpp#L79) after this patch was landed.
U_CAPI const char * U_EXPORT2
udispopt_getGrammaticalCaseIdentifier(UDisplayOptionsGrammaticalCase grammaticalCase) {
if (grammaticalCase >= 0 && grammaticalCase < UPRV_LENGTHOF(grammaticalCaseIds)) {
return grammaticalCaseIds[grammaticalCase];
}
return grammaticalCaseIds[0];
}grammaticalCase is a enum. And whether enum type is unsigned or not does not seem to be strictly defined in C/C++ standard, it ups to the compiler to decide how enum type should be defined. In that case, this check is legit and shouldn't trigger an warning.
I'd move this into bug fixes as I don't think this is enough of a potential disruption to warrant putting it here. If we get user feedback about surprises, we might move it back to here later.