[PATCH] D80514: [clang-tidy] modernize-use-trailing-return-type support for C++20 concepts and decltype

[Bernhard Manfred Gruber via Phabricator via cfe-commits]

bernhardmgruber added a comment.

Great, I seems I forgot to submit the inline comments. Sorry about that!



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:293
+    StringRef File = SM.getBufferData(Loc.first);
+    const char *TokenBegin = File.data() + Loc.second;
+    Lexer Lexer(SM.getLocForStartOfFile(Loc.first), LangOpts, File.begin(),
----------------
aaron.ballman wrote:
> Should we verify that `End` is valid before doing this pointer arithmetic with a value derived from it? For instance, what if `End` points into the scratch buffer because it relies on a macro defined on the command line -- will the logic still work?
That sounded like a scary scenario! Fortunately, `expandIfMacroId` even expands source locations inside the scratch buffer correctly into a location in a real file. I added two tests for it and they seem to pass. Thank you for the hint!


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:430
+      AT->getKeyword() == AutoTypeKeyword::Auto &&
+      !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType()))
+    return;
----------------
aaron.ballman wrote:
> Why do we need to check that there aren't any nested local qualifiers?
Because I would like the check to rewrite e.g. `const auto f();` into `auto f() -> const auto;`. If I omit the check for nested local qualifiers, then those kind of declarations would be skipped.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:285-302
+    SourceLocation End =
+        expandIfMacroId(ReturnLoc.getSourceRange().getEnd(), SM);
+    SourceLocation BeginNameF = expandIfMacroId(F.getLocation(), SM);
+
+    // Extend the ReturnTypeRange until the last token before the function
+    // name.
+    std::pair<FileID, unsigned> Loc = SM.getDecomposedLoc(End);
----------------
aaron.ballman wrote:
> bernhardmgruber wrote:
> > Is there an easier way to get the token previous to the function name?
> There isn't, but if you find you're missing source location information for something, you can also consider modifying Clang to add that source location information and mark this as a dependent patch. It's not clear to me whether that would be worth the effort for this patch or not, but my preference is to avoid these little hacks whenever possible.
Thank you for the hint! I considered this for the concept location in an `AutoReturnType` and the expression inside a `decltype` `AutoReturnType`. Unfortunately I could not wrap my head around what is going on in SemaType.cpp :/


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

https://reviews.llvm.org/D80514