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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Some thoughts.



================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:21-22
+
+// very similar to UseOverrideCheck
+SourceLocation findTrailingReturnTypeLocation(const CharSourceRange &range,
+                                              const ASTContext &ctx,
----------------
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?



================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:72
+
+  llvm_unreachable("Expected to find a closing paranthesis");
+}
----------------
parenthesis


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:75
+
+SourceRange findReturnTypeAndCVRange(const FunctionDecl &f,
+                                     const ASTContext &ctx,
----------------
Same question, really.


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:82
+  // and volatile
+  auto returnTypeRange = f.getReturnTypeSourceRange();
+
----------------
Is this `SourceRange`?


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:91
+  Token t;
+  std::vector<Token> tokens;
+  while (!lexer.LexFromRawLexer(t)) {
----------------
Wouldn't `SmallVector<Token, 4>` be sufficient in most cases?


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:161-163
+  const auto &ctx = *Result.Context;
+  const auto &sm = *Result.SourceManager;
+  const auto &opts = getLangOpts();
----------------
1. All the variables should be WithThisCase
2. Can't tell if the `auto` is ok here? 



================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:165
+
+  auto returnTypeCVRange = findReturnTypeAndCVRange(*f, ctx, sm, opts);
+  if (!returnTypeCVRange.isValid()) {
----------------
It might be better to drop `Range` suffix, and spell the expected return type `SourceRange` explicitly.
Or add `Source` word :)


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:178-179
+    return;
+  const auto insertionLoc =
+      findTrailingReturnTypeLocation(charRange, ctx, sm, opts);
+
----------------
Same, i'd say either add `Source`, or drop `Location` and spell it out completely.


================
Comment at: docs/clang-tidy/checks/modernize-use-trailing-return.rst:4
+modernize-use-trailing-return
+======================
+
----------------
Please pad with extra `=` (to align the length)


================
Comment at: docs/clang-tidy/checks/modernize-use-trailing-return.rst:7
+
+Rewrites function signatures to use a trailing return type.
----------------
Needs better docs, ideally with some examples


================
Comment at: test/clang-tidy/modernize-use-trailing-return.cpp:2
+// RUN: %check_clang_tidy %s modernize-use-trailing-return %t -- -- --std=c++14
+
+namespace std {
----------------
Missing test coverage:
* macros
* is there tests for implicit functions?
* Out-of-line function with body.


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

https://reviews.llvm.org/D56160