-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
[clang-tidy] Avoid checking magic numbers if _BitInt #65888
Conversation
Recent changes to add _BitInt support have caused our internal random testing to fail. This change just avoids a readability magic numbers check for now if a _BitInt. The crash seen (edited for clarity) is shown below. <src-root>/llvm/include/llvm/ADT/APInt.h:1488: uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits() <= 64 && "Too many bits for uint64_t"' failed. ... llvm#9 <address> llvm::APInt::getZExtValue() const <src-root>/llvm/include/llvm/ADT/APInt.h:1488:5 clang::IntegerLiteral const*) const <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47 <clang::IntegerLiteral>(clang::ast_matchers::MatchFinder::MatchResult const&, char const*) <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5 clang::ast_matchers::MatchFinder::MatchResult const&) <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35 ... Reviewed By: donat.nagy
@llvm/pr-subscribers-clang-tidy ChangesRecent changes to add _BitInt support have caused our internal random testing to fail. This change just avoids a readability magic numbers check for now if a _BitInt. The crash seen (edited for clarity) is shown below. /llvm/include/llvm/ADT/APInt.h:1488: ... /llvm/include/llvm/ADT/APInt.h:1488:5 clang::IntegerLiteral const*) const /clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47 clang::IntegerLiteral(clang::ast_matchers::MatchFinder::MatchResult const&, char const*) /clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5 clang::ast_matchers::MatchFinder::MatchResult const&) /clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35 ... Reviewed By: donat.nagyFull diff: https://github.com/llvm/llvm-project/pull/65888.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp index 7f3c2cb9a0434cc..97c20cf200fa21c 100644 --- a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp @@ -191,6 +191,9 @@ bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult &Result, } bool MagicNumbersCheck::isIgnoredValue(const IntegerLiteral *Literal) const { + if (Literal->getType()->isBitIntType()) { + return true; + } const llvm::APInt IntValue = Literal->getValue(); const int64_t Value = IntValue.getZExtValue(); if (Value == 0) diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/bitint-no-crash.c b/clang-tools-extra/test/clang-tidy/checkers/readability/bitint-no-crash.c new file mode 100644 index 000000000000000..f8660924cbef5a0 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/bitint-no-crash.c @@ -0,0 +1,6 @@ +// RUN: %check_clang_tidy %s readability-magic-numbers %t -- + +// Don't crash + +_BitInt(128) A = 4533629751480627964421wb; +// CHECK-MESSAGES: warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
not perfect, as we going to ignore big ints, but still better than crashing for now.
LGTM as a temporary measure. Perhaps add a TODO note which asks for a more through solution; but you can also merge this as it is now. |
Recent changes to add _BitInt support have caused our internal random testing to fail. This change just avoids a readability magic numbers check for now if a _BitInt. The crash seen (edited for clarity) is shown below. <src-root>/llvm/include/llvm/ADT/APInt.h:1488: uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits() <= 64 && "Too many bits for uint64_t"' failed. ... llvm#9 <address> llvm::APInt::getZExtValue() const <src-root>/llvm/include/llvm/ADT/APInt.h:1488:5 clang::IntegerLiteral const*) const <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47 <clang::IntegerLiteral>(clang::ast_matchers::MatchFinder::MatchResult const&, char const*) <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5 clang::ast_matchers::MatchFinder::MatchResult const&) <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35 ... Reviewed By: donat.nagy
Recent changes to add _BitInt support have caused our internal random testing to fail. This change just avoids a readability magic numbers check for now if a _BitInt. The crash seen (edited for clarity) is shown below. <src-root>/llvm/include/llvm/ADT/APInt.h:1488: uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits() <= 64 && "Too many bits for uint64_t"' failed. ... #9 <address> llvm::APInt::getZExtValue() const <src-root>/llvm/include/llvm/ADT/APInt.h:1488:5 clang::IntegerLiteral const*) const <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47 <clang::IntegerLiteral>(clang::ast_matchers::MatchFinder::MatchResult const&, char const*) <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5 clang::ast_matchers::MatchFinder::MatchResult const&) <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35 ... Reviewed By: donat.nagy
Recent changes to add _BitInt support have caused our internal random testing to fail. This change just avoids a readability magic numbers check for now if a _BitInt. The crash seen (edited for clarity) is shown below. <src-root>/llvm/include/llvm/ADT/APInt.h:1488: uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits() <= 64 && "Too many bits for uint64_t"' failed. ... #9 <address> llvm::APInt::getZExtValue() const <src-root>/llvm/include/llvm/ADT/APInt.h:1488:5 clang::IntegerLiteral const*) const <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47 <clang::IntegerLiteral>(clang::ast_matchers::MatchFinder::MatchResult const&, char const*) <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5 clang::ast_matchers::MatchFinder::MatchResult const&) <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35 ... Reviewed By: donat.nagy
Recent changes to add _BitInt support have caused our internal random testing to fail. This change just avoids a readability magic numbers check for now if a _BitInt. The crash seen (edited for clarity) is shown below.
/llvm/include/llvm/ADT/APInt.h:1488:
uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits()
<= 64 && "Too many bits for uint64_t"' failed.
...
llvm::APInt::getZExtValue() const#9
/llvm/include/llvm/ADT/APInt.h:1488:5
clang::IntegerLiteral const*) const
/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47
clang::IntegerLiteral(clang::ast_matchers::MatchFinder::MatchResult
const&, char const*)
/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5
clang::ast_matchers::MatchFinder::MatchResult const&)
/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35
...
Reviewed By: donat.nagy