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