[clang-tidy] Fix bug in readability-uppercase-literal-suffix

[carlosgalvezp]

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=51790. The check triggers incorrectly with non-type template parameters.

A bisect determined that the bug was introduced here:
https://github.com/llvm/llvm-project/commit/ea2225a10be986d226e041d20d36dff17e78daed

Unfortunately that patch can no longer be reverted on top of the main branch, so add a fix instead. Add a unit test to avoid regression in the future.

[carlosgalvezp]

I personally think this fix should happen when doing the match, not when analyzing the matches. This is my first patch to LLVM and I'm not knowledgeable in AST so I don't really know how to go about that :) Please come with suggestions if there's a better way to do this!

[aaron.ballman]

Thank you for the fix!

clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp

137–138

clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp

286

Can you add a // CHECK-MESSAGES-NOT: line to the places where we used to diagnose but no longer do? That helps make the test more explicit.

Another test to add is a nontype template parameter whose identifier ends in a digit just before the i. e.g., foo1i

[carlosgalvezp]

Fixed review comments

[aaron.ballman]

LGTM, thank you! Would you like me to commit on your behalf? If so, please let me know what name and email address you'd like me to use for patch attribution in git.

[carlosgalvezp]

Awesome, thanks a lot for the review! :) Sure, you can use the following to commit on my behalf, since I don't think I have such permissions:

Carlos Galvez
[e-mail]

[aaron.ballman]

In D110493#3025343, @carlosgalvezp wrote:

Awesome, thanks a lot for the review! :) Sure, you can use the following to commit on my behalf, since I don't think I have such permissions:

Carlos Galvez
[e-mail]

Thanks! I've commit on your behalf in b2a2c38349a18b89b04d342632d5ea02f86dfdd6.

[carlosgalvezp]

Super, thanks! It went really smooth :)

Newbie question: I noticed that [clang-tidy] is no longer part of the commit message, I thought the intention was to keep it to more easily glance through the commit history?

[aaron.ballman]

In D110493#3025394, @carlosgalvezp wrote:

Super, thanks! It went really smooth :)

Newbie question: I noticed that [clang-tidy] is no longer part of the commit message, I thought the intention was to keep it to more easily glance through the commit history?

We usually put [project] markings in the subject so it's easier for code reviewers to know what PRs are in an area of interest to them, and I suppose I could keep them as part of the commit message. But the commit message "title" is supposed to be concise (I hear 50 characters is a common measure for it), so I usually remove the [project] marker reflexively to keep the title of the commit brief.

Btw, this patch failed some bots because of a missing #include which I've added in ef0f728abe6ec43f2f75082c9b47ec7fade2ead2.

[carlosgalvezp]

Thanks for the clarification, makes sense! I got the notification about the build failure, thanks for fixing. I actually added it in the first place but then noticed that checkers typically didn't include any STL header so I thought maybe there was a restriction on their usage or something. The code compiled just fine when running the "clang-check-tools" target. Good to know for the next time :)

PS: I've also reported the build errors in pre-merge, maybe they would have caught it earlier.

clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp

shouldReplaceLiteralSuffix(const Expr &Literal,
   // Else keep the naive literal location!
 
   // Get the whole literal from the source buffer.
   bool Invalid;
   const StringRef LiteralSourceText = Lexer::getSourceText(
       CharSourceRange::getTokenRange(*Range), SM, LO, &Invalid);
   assert(!Invalid && "Failed to retrieve the source text.");
 
+  // Make sure the first character is actually a digit, instead of
+  // something else, like a non-type template parameter.
+  if (!std::isdigit(static_cast<unsigned char>(LiteralSourceText.front())))
+    return llvm::None;
+
   size_t Skip = 0;
 
   // Do we need to ignore something before actually looking for the suffix?
   if (!LiteralType::SkipFirst.empty()) {
     // E.g. we can't look for 'f' suffix in hexadecimal floating-point literals
     // until after we skip to the exponent (which is mandatory there),
     // because hex-digit-sequence may contain 'f'.
     Skip = LiteralSourceText.find_first_of(LiteralType::SkipFirst);

clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp

 namespace {
 enum a { b };
 constexpr bool operator&(a, a) { return int(); }
 template <a l>
 void c() { l &a(); }
 void d();
 void d() { c<b>(); }
 } // namespace
+
+// Check that non-type template parameters do not cause any diags.
+// https://bugs.llvm.org/show_bug.cgi?id=51790
+template <int capacity>
+struct Vector {
+  static constexpr int kCapacity = capacity;
+};
+
+template <int capacity>
+constexpr int Vector<capacity>::kCapacity;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:22: warning: integer literal has suffix 'ity', which is not uppercase
+
+template <int foo1u>
+struct Foo {
+  static constexpr int kFoo = foo1u;
+};
+
+template <int foo1u>
+constexpr int Foo<foo1u>::kFoo;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:19: warning: integer literal has suffix 'u', which is not uppercase
+
+// The template needs to be instantiated for diagnostics to show up
+void test_non_type_template_parameter() {
+  int x = Vector<10>::kCapacity;
+  int f = Foo<10>::kFoo;
+}