lebedev.ri added inline comments.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:33
+};
+constexpr llvm::StringLiteral IntegerLiteral::Name;
+constexpr llvm::StringLiteral IntegerLiteral::SkipFirst;
----------------
JonasToth wrote:
> why are these declarations necessary?
Well, because these are *declarations*.
Else the linker complains, since i'm using these in non-`constexpr` context,
but only provided definitions.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:42
+ // What should be skipped before looking for the Suffixes?
+ // Hexadecimal floating-point literals: skip until exponent first.
+ static constexpr llvm::StringLiteral SkipFirst = llvm::StringLiteral("pP");
----------------
JonasToth wrote:
> The second line of the comment is slightly confusing, please make a full
> sentence out of it.
Rewrote, hopefully better?
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:52
+AST_MATCHER(clang::IntegerLiteral, intHasSuffix) {
+ const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr());
+ if (!T)
----------------
JonasToth wrote:
> as it hit me in my check: what about `(1)ul`? Is this syntactically correct
> and should be diagnosed too? (Please add tests if so).
>
> In this case it should be `Note.getType().IgnoreParens().getTypePtr())`.
clang says invalid
https://godbolt.org/z/R8bGe_
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:53
+ const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr());
+ if (!T)
+ return false;
----------------
JonasToth wrote:
> Maybe the if could init `T`? It would require a second `return false;` if i
> am not mistaken, but looks more regular to me. No strong opinion from my side.
Then we will not have an early-return, which is worse than this.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:85
+template <typename LiteralType>
+llvm::Optional<UppercaseLiteralSuffixCheck::NewSuffix>
+UppercaseLiteralSuffixCheck::shouldReplaceLiteralSuffix(
----------------
JonasToth wrote:
> These types get really long. Is it possible to put `NewSuffix` into the
> anonymous namespace as well?
No, because `shouldReplaceLiteralSuffix()` is a member function which returns
that type.
I think it should stay a member function, so theoretically `NewSuffix` could be
a [second] template param, but that is kinda ugly..
I also can't simplify it via `using` because the `NewSuffix` is private.
Perhaps we should keep this as-is?
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:95
+ // Get the whole Integer Literal from the source buffer.
+ const StringRef LiteralSourceText = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(S.Range), SM, getLangOpts());
----------------
JonasToth wrote:
> Please check if the source text could be retrieved, with a final `bool`
> parameter, thats in/out and at least `assert` on that.
I looked at a randomly-selected few dozen calls to this function within
clang-tidy, and none of those do this.
But `assert` added, better than nothing.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:129
+ if (S.OldSuffix == S.NewSuffix)
+ return llvm::None; // The suffix was already fully uppercase.
+
----------------
JonasToth wrote:
> Could this function return a `Optional<FixitHint>`? That would include the
> range and the relacement-text. I feel that is would simplify the code,
> especially the amount of state that one has to keep track of.
I still want to see the old suffix, but i think we can form the FixitHint here,
and store it.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:135
+void UppercaseLiteralSuffixCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ stmt(integerLiteral(intHasSuffix())).bind(IntegerLiteral::Name), this);
----------------
JonasToth wrote:
> I think you can merge this matcher to
> `stmt(eachOf(integerLiteral(intHasSuffix()).bind(),
> floatLiteral(fpHasSuffix()).bind()))`
>
> `eachOf` because we want to match all, and not short-circuit.
`bind()` still wants the name though.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:152
+ // Ignore literals that aren't fully written in the source code.
+ if (LiteralLocation.isMacroID() ||
+ Result.SourceManager->isMacroBodyExpansion(LiteralLocation) ||
----------------
JonasToth wrote:
> Wouldn't it make sense to warn for the literals in macros?
Very obscure example (like all macros is), but i think it shows the point:
```
#include <stdio.h>
#include <stdio.h>
#define xstr(s) str(s)
#define str(s) #s
#define dump(X) printf("%u is " str(X) "nits", X);
int main () {
dump(1u);
return 0;
}
```
will normally print `1 is 1units`
But if you uppercase it, it will print `1 is 1Units`.
I would honestly prefer to give macros a pass here..
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165
+
+ diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case")
+ << LiteralType::Name << S.OldSuffix
----------------
JonasToth wrote:
> JonasToth wrote:
> > Lets move this `diag` into the true branch of the if stmt and drop the
> > ´else`.
> I think the warning should point at the suffix, which can be retrieved from
> the replacement range. Then you don't need to include the suffix itself in
> the diagnostic
If the warnings are aggregated (i.e. not raw `make` dump), with only the first
line shown, the suffix will still be invisible.
Regarding the pointer direction, i'm not sure.
For now i have reworded the diag to justify pointing at the literal itself.
================
Comment at: docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst:9
+Detects when the integral literal or floating point (decimal or hexadecimal)
+literal has non-uppercase suffix, and suggests to make the suffix uppercase,
+with fix-it.
----------------
JonasToth wrote:
> I feel that the sentence could be improved a bit.
>
> - `literal has *a* non-uppercase`
> - `suffix and provides a fix-it-hint with the uppercase suffix.` (i think the
> comma after `suffix` is not necessary)
Words are the hardest part :)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
