[clang-tidy] Magic number checker shouldn't warn on user defined string literals

[bruntib]

Summary

The following code snippet results a false positive report in the magic number checker:

std::string s = "Hello World"s;

The expression "Hello World"s has a StringLiteral in its AST, since this is a function call on std::string operator "" s(const char*, std::size_t) and the second parameter is passed the length of the string literal.

This patch fixes https://bugs.llvm.org/show_bug.cgi?id=40633

[lebedev.ri]

clang-tools-extra/test/clang-tidy/readability-magic-numbers.cpp

1 ↗

(On Diff #219065)

IIRC %check_clang_tidy is actually run for every standard, so this decreases test coverage;
just add a c++14-specific test.

[0x8000-0000]

Looks good to me. Thank you for the fix.

[bruntib]

Thank you for the suggestion. I didn't know about this behavior of %check_clang_tidy.

[Eugene.Zelenko]

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

125

You could use const auto *, because type is spelled in same statement.

[bruntib]

Usually I'm not a big fan of auto, but in this case it makes sense not to duplicate the type name in the same expression. I'll use this rule of thumb in the future, thanks :)

[0x8000-0000]

@aaron.ballman - can you please review this patch and if you find it acceptable please integrate it?

[aaron.ballman]

Thanks for the patch, I've commit in be7d633a6fa6ddae6b7f455f5f336555d088c62d

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

void MagicNumbersCheck::check(const MatchFinder::MatchResult &Result) {
   checkBoundMatch<FloatingLiteral>(Result, "float");
 }
 
 bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult &Result,
                                    const Expr &ExprResult) const {
   return llvm::any_of(
       Result.Context->getParents(ExprResult),
       [&Result](const DynTypedNode &Parent) {
-        return isUsedToInitializeAConstant(Result, Parent) ||
-               // Ignore this instance, because this match reports the location
-               // where the template is defined, not where it is instantiated.
-               Parent.get<SubstNonTypeTemplateParmExpr>();
+        if (isUsedToInitializeAConstant(Result, Parent))
+          return true;
+
+        if (Parent.get<SubstNonTypeTemplateParmExpr>())
+          return true;
+
+        // Don't warn on string user defined literals:
+        // std::string s = "Hello World"s;
+        if (const auto *UDL = Parent.get<UserDefinedLiteral>())
+          if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
+            return true;
+
+        return false;
       });
 }
 
 bool MagicNumbersCheck::isIgnoredValue(const IntegerLiteral *Literal) const {
   const llvm::APInt IntValue = Literal->getValue();
   const int64_t Value = IntValue.getZExtValue();
   if (Value == 0)
     return true;

clang-tools-extra/test/clang-tidy/readability-magic-numbers-userliteral.cpp

+// RUN: %check_clang_tidy -std=c++14-or-later %s readability-magic-numbers %t --
+
+namespace std {
+  class string {};
+  using size_t = decltype(sizeof(int));
+  string operator ""s(const char *, std::size_t);
+  int operator "" s(unsigned long long);
+}
+
+void UserDefinedLiteral() {
+  using std::operator ""s;
+  "Hello World"s;
+  const int i = 3600s;
+  int j = 3600s;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3600s is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}