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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
