[PATCH] D56160: [clang-tidy] modernize-use-trailing-return check

[Jonas Toth via Phabricator via cfe-commits]

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