Page MenuHomePhabricator

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

Authored by bruntib on Sep 6 2019, 5:19 AM.

Details

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

Diff Detail

Event Timeline

bruntib created this revision.Sep 6 2019, 5:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2019, 5:19 AM
lebedev.ri added inline comments.
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 accepted this revision.Sep 6 2019, 5:45 AM

Looks good to me. Thank you for the fix.

This revision is now accepted and ready to land.Sep 6 2019, 5:45 AM
bruntib updated this revision to Diff 219088.Sep 6 2019, 6:18 AM
bruntib edited the summary of this revision. (Show Details)

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

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
125

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

bruntib updated this revision to Diff 219097.Sep 6 2019, 7:14 AM

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 :)

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

aaron.ballman closed this revision.Dec 9 2019, 10:13 AM

Thanks for the patch, I've commit in be7d633a6fa6ddae6b7f455f5f336555d088c62d