JonasToth added inline comments.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:33
+ const LangOptions &LangOpts) {
+ // we start with the location of the closing parenthesis.
+ const TypeSourceInfo *TSI = F.getTypeSourceInfo();
----------------
Nit: s/we/We
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:39
+ }
+ const FunctionTypeLoc FTL =
+ TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>();
----------------
`const` is omitted in LLVM for value-types, only references and pointers are
annottated with it.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:46
+
+ // if the function argument list ends inside of a macro, it is dangerous to
+ // start lexing from here - bail out.
----------------
Nit: s/if/If/
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:48
+ // start lexing from here - bail out.
+ const SourceLocation ClosingParen = FTL.getRParenLoc();
+ if (ClosingParen.isMacroID()) {
----------------
same `const` argument.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:57
+
+ // skip subsequent CV and ref qualifiers.
+ std::pair<FileID, unsigned> Loc = SM.getDecomposedLoc(Result);
----------------
Nit: s/skip/Skip/
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:87
+
+ // we start with the range of the return type and expand to neighboring const
+ // and volatile.
----------------
Nit: s/we/We/, s/const/'const'/, s/volatile/'volatile'/
We aim to highlight code-constructs to emphasize we are talking about the
construct and not confuse the meaing of words.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:91
+ if (ReturnTypeRange.isInvalid()) {
+ diag(F.getLocation(), Message); // happens if e.g. clang cannot resolve all
+ // includes and the return type is unknow.
----------------
I think this comment does not add value? Logically it makes sense to not return
an invalid range and not work further with it regardless of reason for invalid
range.
Anyhow, i would prefer a non-trailing comment.
s/happens/Happens/, s/unknow/unknown/ typo.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:96
+
+ // if the return type has no local qualifiers, it's source range is accurate.
+ if (!F.getReturnType().hasLocalQualifiers())
----------------
Nit: s/if/If/ (stop commenting on these now :P)
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:100
+
+ const SourceLocation BeginF = expandIfMacroId(F.getBeginLoc(), SM);
+ const SourceLocation BeginNameF = expandIfMacroId(F.getLocation(), SM);
----------------
please ellide this `const`, as above
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:118
+ if (Info.hasMacroDefinition()) {
+ diag(F.getLocation(), Message); // CV qualifiers in macros
+ return {};
----------------
Please improve that comment and here I would prefer a non-trailing comment, too.
Especially formulate whats with CV and macros, the meaning has to be guessed
(and can be guessed wrong).
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:133
+ bool ExtendedLeft = false;
+ for (size_t i = 0; i < Tokens.size(); i++) {
+ // if we found the beginning of the return type, include const and volatile
----------------
please use uppercase `i` and `j` names for consistency.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:136
+ // to the left.
+ if (!SM.isBeforeInTranslationUnit(Tokens[i].getLocation(),
+ ReturnTypeRange.getBegin()) &&
----------------
The whole function is quite complex, involved in multiple things (lexing,
diagnostics, macro-stuff) and not easy to understand.
Could you please take a look at it again and try to split it up in smaller
functions?
It looks a bit fragile and parts of it might deserve separate unit-tests.
Clarifying these parts of the code is definitly worth it.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:162
+ functionDecl(unless(anyOf(hasTrailingReturn(), returns(voidType()),
+ returns(autoType()), cxxConversionDecl(),
+ cxxMethodDecl(isImplicit()))))
----------------
Shouldn't you include `returns(decltypeType())` as well?
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:176
+ // TODO: implement those
+ if (F->getDeclaredReturnType()->isFunctionPointerType() ||
+ F->getDeclaredReturnType()->isMemberFunctionPointerType() ||
----------------
Please add these as `Limitations` to the user-facing documentation. Some other
checks do the same, just grep a bit for an example-
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:187
+
+ const SourceLocation InsertionLoc =
+ findTrailingReturnTypeSourceLocation(*F, Ctx, SM, LangOpts);
----------------
drop const
================
Comment at: test/clang-tidy/modernize-use-trailing-return.cpp:269
+auto l1 = [](int arg) {};
+auto l2 = [](int arg) -> double {};
----------------
These tests are missing the great template fun :)
Lets start with those two examples:
```
template <typename Container>
[[maybe_unused]] typename Container::value_type const volatile&&
myFunnyFunction(Container& C) noexcept;
```
and
```
#define MAYBE_UNUSED_MACRO [[maybe_unused]]
template <typename Container>
MAYBE_UNUSED_MACRO typename Container::value_type const volatile** const
myFunnyFunction(Container& C) noexcept;
```
Its not necessarily nice code, but I am sure something like this is somewhere
in boost for example ;)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56160/new/
https://reviews.llvm.org/D56160
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
