[PATCH] D56160: [clang-tidy] modernize-use-trailing-return check
Jonas Toth via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jan 7 09:47:17 PST 2019
JonasToth added a comment.
> - if the closing parenthesis of the function is inside a macro, no FixIt will be created (I cannot relyably lex for subsequent CV and ref qualifiers and maybe we do not want to make changes in macros)
Usually macros are untouched because its impossible to transform them correctly. Someone, somewhere does evil stuff with them and is of course mad if the transformation interfers ;)
> - if the return type is not CV qualified, i allow macros to be part of it because no lexing is required
> - if the return type is CV qualified and contains macros, I provide no FixIt
>
> I improved findTrailingReturnTypeSourceLocation() by discovering FunctionTypeLoc::getRParenLoc(). I still require some lexing for CV and ref qualifiers though.
>
> I tried to improve findReturnTypeAndCVSourceRange() by finding a way to get the source range directly from the AST, but it seems the AST does not store source locations for CV qualifiers of types. I read that in the docs for QualifiedTypeLoc <https://clang.llvm.org/doxygen/classclang_1_1QualifiedTypeLoc.html#details>. So it seems I cannot circumvent finding the locations of const and volatile by my own.
Yup.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:161-163
+ const auto &ctx = *Result.Context;
+ const auto &sm = *Result.SourceManager;
+ const auto &opts = getLangOpts();
----------------
bernhardmgruber wrote:
> 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.
`auto` shuold only be used if its obvious what type the variable has (e.g. `auto foo = make_unique<MyClass>()`, but not for `auto Foo = GetSomeResult();`).
This makes review and reading easier and is the guideline for LLVM. It has room for subjective reasoning, but adding the type is usually better.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:19
+namespace modernize {
+constexpr auto Message = "use a trailing return type for this function";
+
----------------
`auto -> const char*` here, thats not nice :)
How about a `llvm::StringRef` or https://llvm.org/doxygen/classllvm_1_1StringLiteral.html (in this case better)
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:33
+ const TypeSourceInfo *TSI = F.getTypeSourceInfo();
+ assert(TSI);
+ const FunctionTypeLoc FTL =
----------------
Please add an error-msg to the assertion like `assert(TSI && "Reason why this must hold");`. Humanreadable for debugging.
================
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
----------------
Please make that comment (and all comments in general) full grammatical sentence with correct punctuation.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:42
+ if (ClosingParen.isMacroID()) {
+ diag(
+ F.getLocation(),
----------------
weird formatting, did clang-format do that?
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:45
+ std::string(Message) +
+ " (no FixIt provided, function argument list end is inside macro)");
+ return {};
----------------
I think you can ellide that extra message. Not emitting the fixit is clear already.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:46
+ " (no FixIt provided, function argument list end is inside macro)");
+ return {};
+ }
----------------
Indiciator to use `llvm::Optional` as return type instead. What do you think?
================
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);
----------------
Values are usually not `const`'ed. Please change that for consistency.
================
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)) {
----------------
why are pointers not relevant here?
There should be `Token.oneOf()` or `Token.isOneOf()` or similar for this usecase
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:88
+ diag(F.getLocation(),
+ std::string(Message) + " (failed to determine return type source "
+ "range, could clang resolve all #includes?)",
----------------
Same with the other extra-bit of information. This will rather confuse the user of clang-tidy then give additional information. Please ellide it.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:102
+ // create tokens for everything before the name of the function
+ const std::pair<FileID, unsigned> Loc = SM.getDecomposedLoc(BeginF);
+ const StringRef File = SM.getBufferData(Loc.first);
----------------
please ellide const here
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:117
+ diag(F.getLocation(), std::string(Message) +
+ " (no FixIt provided, you have CV qualifiers "
+ "in macros around your return type)");
----------------
message.
You can use `llvm::StringRef` instead of `std::string` (here and elsewhere) as you don't need to own the message (after removing the extra part).
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:128
+ // include const and volatile to the left and right of the return type
+ auto IsCV = [](Token T) {
+ return T.is(tok::kw_const) || T.is(tok::kw_volatile);
----------------
`Token` has a special method to check for one of many tokenkinds, please use that instead.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:170
+ const auto *F = Result.Nodes.getNodeAs<FunctionDecl>("f");
+ if (!F)
+ return;
----------------
you can assert here, as landing here means the matcher fired.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:173
+
+ if (F->getDeclaredReturnType()->isFunctionPointerType()) {
+ diag(F->getLocation(), std::string(Message) +
----------------
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.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:175
+ diag(F->getLocation(), std::string(Message) +
+ " (no FixIt provided, function pointer return "
+ "types are not implemented)");
----------------
message.
================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:197
+ // space before & ...
+ const StringRef ReturnType = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(ReturnTypeCVRange), SM, LangOpts);
----------------
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
================
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]
+
+
----------------
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.
================
Comment at: test/clang-tidy/modernize-use-trailing-return.cpp:173
+auto l1 = [](int arg) {};
+auto l2 = [](int arg) -> double {};
----------------
bernhardmgruber wrote:
> JonasToth wrote:
> > you could figure out the return type of the lambda if it contains a return, otherwise it should be `void`.
> I am sorry, but I do not understand what you want. Lambdas have trailing return types by default (if it is not left out and deduced). Do you want to explicitely generate the deduced return type? This is not what I intended. I want to rewrite old return types on the left to be trailing.
I understood the check in the way, that the trailing return type would be added always.
It is ok to leave lambdas alone if that is not what you intent.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56160/new/
https://reviews.llvm.org/D56160
More information about the cfe-commits
mailing list