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

Mon Jan 7 18:02:09 PST 2019

bernhardmgruber added inline comments.

================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:33
+  const TypeSourceInfo *TSI = F.getTypeSourceInfo();
+  assert(TSI);
+  const FunctionTypeLoc FTL =
----------------
JonasToth wrote:
> Please add an error-msg to the assertion like `assert(TSI && "Reason why this must hold");`. Humanreadable for debugging.
I replaced the asserts by an error with a message. Honestly, I do not know when these might be nullptr. It just never occured during my tests.

================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:38
+
+  // if the function argument list ends inside of a macro, it is dangerous to
+  // start lexing from here, bail out
----------------
JonasToth wrote:
> Please make that comment (and all comments in general) full grammatical sentence with correct punctuation.
I am not a native English speaker, but the only thing that might be odd about this sentence is that `, bail out`. Maybe a em-dash is what we need here, but this not Latex. Oh yes, and inside of a macro is also weird.

I will make it: if the function argument list ends inside a macro, it is dangerous to start lexing from here - bail out.

I will also add dots at the end of sentences, other checks seem to follow this convention (Personally, I do not like the punctuation at the end, as it does not add significant information)

================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:42
+  if (ClosingParen.isMacroID()) {
+    diag(
+        F.getLocation(),
----------------
JonasToth wrote:
> weird formatting, did clang-format do that?
I use the latest version of Visual Studio and it picks up .clang-format files and formats automatically when I save. I can try to run clang-format by hand.

================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:45
+        std::string(Message) +
+            " (no FixIt provided, function argument list end is inside macro)");
+    return {};
----------------
JonasToth wrote:
> I think you can ellide that extra message. Not emitting the fixit is clear already.
I actually like having a reason why my check could not provide a FixIt. Especially because there are multiple reasons why this might happen.

================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:46
+            " (no FixIt provided, function argument list end is inside macro)");
+    return {};
+  }
----------------
JonasToth wrote:
> Indiciator to use `llvm::Optional` as return type instead. What do you think?
I absolutely agree that optional is a great type. But SourceLocation has a sentinel/empty value, which can be queried using `Valid()/Invalid()`. SourceLocation has the optionality built in so to speak. I have also seen other checks using a default constructed source location to indicate no result. Using optional would make the function interface more clear, but then we could have the weird case of an initialized optional containing an invalid source location. I would like to leave it as it is.

================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:53
+  // skip subsequent CV and ref qualifiers
+  const std::pair<FileID, unsigned> Loc = SM.getDecomposedLoc(Result);
+  const StringRef File = SM.getBufferData(Loc.first);
----------------
JonasToth wrote:
> Values are usually not `const`'ed. Please change that for consistency.
I do not agree to have something mutable, that should not change. Especially, now that everything else around is const, I would be suspicious why the writer had not put a const here and where the variable is modified after its initialization. But if you require it for consistency, I can remove it.

================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:67
+
+    if (T.is(tok::amp) || T.is(tok::ampamp) || T.is(tok::kw_const) ||
+        T.is(tok::kw_volatile)) {
----------------
JonasToth wrote:
> why are pointers not relevant here?
> There should be `Token.oneOf()` or `Token.isOneOf()` or similar for this usecase
I am lexing CV and ref qualifiers after the closing parenthesis of a function's argument list (i.e. a member function then). I do not know what you mean with pointers here. An asterisk cannot appear as a qualifier on a method AFAIK.

Thanks for the tip!

================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:173
+
+  if (F->getDeclaredReturnType()->isFunctionPointerType()) {
+    diag(F->getLocation(), std::string(Message) +
----------------
JonasToth wrote:
> What about data-pointer-member types? (https://en.cppreference.com/w/cpp/language/pointer point 2))
> Its an uncommon construct, but better catch it here instead of bug-reports.
Those are fine, because they do not span space before and behind the function name and argument list when declaring them. But member function pointer types also needed to be excluded. Thank you for leading me to this test case!

Edit: I just found out that the return type source range given by the AST does only cover the first token of the qualifier to the pointer:

```
int std::string::* e4();
~~~~~~~
```
I have to investigate this.

================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:197
+  // space before & ...
+  const StringRef ReturnType = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(ReturnTypeCVRange), SM, LangOpts);
----------------
JonasToth wrote:
> Maybe `https://clang.llvm.org/doxygen/namespaceclang_1_1tooling_1_1fixit.html#aebb060ec807563c615d5e004b1f6467d` `getText` is slightly cleaner here? (it does exactly your call, but shorter)
> 
> no `const` for StringRef
thanks for the tip!

================
Comment at: test/clang-tidy/modernize-use-trailing-return.cpp:222
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use a trailing return type for this function (no FixIt provided, you have CV qualifiers in macros around your return type) [modernize-use-trailing-return]
+
+
----------------
JonasToth wrote:
> What about this case and variations.
> 
> ```
> template <typename T>
> using Real = T;
> #define PRECISION float
> Real<PRECISION> foo() { return 0.; }
> ```
> 
> Given you do some lexing I want to push that implementation a bit and try to find corner-cases.
works, the macro is inside the return type source range that clang already gives me

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56160/new/

https://reviews.llvm.org/D56160