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

[Bernhard Manfred Gruber via Phabricator via cfe-commits]

bernhardmgruber marked 10 inline comments as done.
bernhardmgruber added a comment.

I fixed most of the stylistic issues. Regarding the missing test cases, I will add those and do the necessary code changes. Thank you very much for pointing them out to me!



================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:21-22
+
+// very similar to UseOverrideCheck
+SourceLocation findTrailingReturnTypeLocation(const CharSourceRange &range,
+                                              const ASTContext &ctx,
----------------
lebedev.ri wrote:
> This function looks quite fragile.
> Is there no existing function elsewhere [in the support libraries] that does this?
> There is no way to extract this from AST?
> 
I have not found one by browsing the doxygen documentation. I got inspired by ParseTokens in UseOverrideCheck.cpp. But I am an absolute beginner on the LLVM codebase, so please point me in a direction and I try to find something better!

There is also this in the documentation of [[ https://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html#a1ac2b87b937cc44d8980a898851c0c75 | FunctionDecl::getReturnTypeSourceRange()]]: "This may omit qualifiers and other information with limited representation in the AST."
So maybe the AST really does not contain the locations of const/volatile and const/ref qualifiers


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:91
+  Token t;
+  std::vector<Token> tokens;
+  while (!lexer.LexFromRawLexer(t)) {
----------------
lebedev.ri wrote:
> Wouldn't `SmallVector<Token, 4>` be sufficient in most cases?
I believe return types with namespaces or template arguments produce more tokens, so I will go with SmallVector<Token, 8>. But i did not do any measurements.


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:161-163
+  const auto &ctx = *Result.Context;
+  const auto &sm = *Result.SourceManager;
+  const auto &opts = getLangOpts();
----------------
lebedev.ri wrote:
> 1. All the variables should be WithThisCase
> 2. Can't tell if the `auto` is ok here? 
> 
1. done
2. Is it important to know what types Result.Context and the others are? I am just passing them on to further functions because I have to, they are not relevant for the logic I am coding.


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

https://reviews.llvm.org/D56160