[clang-tidy] modernize-use-digit-separator issue #35414 #76153
Conversation
# Conflicts: # clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp # clang-tools-extra/docs/ReleaseNotes.rst
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I think I should tag @PiotrZSL. |
| std::string | ||
| getFormatedScientificFloatString(const llvm::StringRef OriginalLiteralString); | ||
|
|
||
| std::vector<std::basic_string<char>> |
There was a problem hiding this comment.
Avoid std::vector, use things like llvm::SmallVector, it will work fine with return value optimization here.
|
|
||
| using namespace clang::ast_matchers; | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
free standing functions should be "static" instead of in anonymous namespace, per codding guidelines
| std::vector<std::basic_string<char>> | ||
| splitStringByGroupSize(const std::basic_string<char> &String, | ||
| size_t GroupSize) { | ||
| std::vector<std::basic_string<char>> Result; |
There was a problem hiding this comment.
do not use std::basic_string, use std::string.
| for (size_t I = 0; I < ReversedString.size(); I += GroupSize) { | ||
| Result.push_back(ReversedString.substr(I, GroupSize)); | ||
| } | ||
|
|
||
| std::reverse(Result.begin(), Result.end()); | ||
| std::for_each(Result.begin(), Result.end(), [](std::basic_string<char> &Str) { | ||
| return std::reverse(Str.begin(), Str.end()); | ||
| }); | ||
|
|
||
| return Result; |
There was a problem hiding this comment.
i do not like this algorithm, is not efficient. work something out without multiple copies,.
| Radix = 2; | ||
| GroupSize = 4; | ||
| Prefix = "0b"; | ||
| } else if (OriginalLiteralString.starts_with("0x")) { |
There was a problem hiding this comment.
what if it starts with 0X ? or ends with 0B
| /// | ||
| /// For the user-facing documentation see: | ||
| /// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-digit-separator.html | ||
| class UseDigitSeparatorCheck : public ClangTidyCheck { |
There was a problem hiding this comment.
Restrict this check to C++14 and later.
| - hexadecimal integral constants, groups of 4 digits, e.g. unsigned long | ||
| x = 0xffff'0000; | ||
| - floating-point constants, group into 3 digits on either side of the | ||
| decimal point, e.g. float x = 3'456.001'25f; |
There was a problem hiding this comment.
add examples in a form of source code.
| float PostfixScientificShortFloat1 = 1.23e10f; | ||
| float PostfixScientificShortFloat2 = 1.23E+10f; | ||
| float PostfixScientificShortFloat3 = 1.23e-10F; | ||
|
|
There was a problem hiding this comment.
add tests where number is behind macro, and where number is an macro argument.
| float PostfixScientificShortFloat2 = 1.23E+10f; | ||
| float PostfixScientificShortFloat3 = 1.23e-10F; | ||
|
|
||
|
|
There was a problem hiding this comment.
Add tests where CHECK-FIXES verify entire source line, to make sure that ; or = is not removed.
| @@ -0,0 +1,239 @@ | ||
| // RUN: %check_clang_tidy %s modernize-use-digit-separator %t |
There was a problem hiding this comment.
restrict tests to std standards that support this notation
|
|
||
| int NotFormattedInteger = 1234567; | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: unformatted representation of integer literal '1234567' [modernize-use-digit-separator] | ||
| // CHECK-FIXES: 1'234'567 |
There was a problem hiding this comment.
Please add a test case like this:
int OldStyleFormattedInteger = 12*1000*1000;
// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: legacy formatted representation of integer literal '12*1000*1000' [modernize-use-digit-separator]
// CHECK-FIXES: 12'000'000
See #35414 (comment) for more details
There was a problem hiding this comment.
I disagree with this suggestion; see my comments on the original issue. A check should have a single responsibility, not two.
Closes #35414
The check that looks for long integral constants and inserts the digits separator (') appropriately. Groupings: