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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D56160#1367811 <https://reviews.llvm.org/D56160#1367811>, @JonasToth wrote:

> In D56160#1367074 <https://reviews.llvm.org/D56160#1367074>, @bernhardmgruber wrote:
>
> > Thank you again @JonasToth for all your valueable input! I could almost successfully run my check on the llvm/lib subfolder. I created a compilation database from within Visual Studio using an extension called SourceTrail <https://www.sourcetrail.com/blog/export_clang_compilation_database_from_visual_studio_solution/>. One of the issues was the following:
>
>
> Very good!
>
> >   struct Object { long long value; };
> >   class C {
> >     int Object;
> >     struct Object m();
> >   };
> >   Object C::m() { return {0}; }
> > 
> > 
> > If I rewrite this to the following
> > 
> >   struct Object { long long value; };
> >   class C {
> >     int Object;
> >     struct Object m();
> >   };
> >   auto C::m() -> Object { return {0}; }
> > 
> > 
> > a compilation error occurs afterwards, because Object now refers to the class member. I discovered a similar problem colliding with the name of a function argument. So essentially, I believe I now require a name lookup of the return type (if it is unqualified) in the scope of the function. In case such a name already exists, i have to prefix `struct/class` or perform no replacement. I looked at the documentation of `ASTContext`, but it seems all the good stuff is in `DeclContext`, which I have not available. How can I perform a name lookup inside the `check` member function?
>
> That is very interesting and I was not aware of this possibility :D
>
> Every `Decl` derives from `DeclContext`, see for example the docs for `CXXMethodDecl` (https://clang.llvm.org/doxygen/classclang_1_1CXXMethodDecl.html)
>  You should be able to look up the names you are interested. I don't know of a good way to check for the issue we have right now, @aaron.ballman knows that probably better then I do.
>  Try to experiment, but If you don't find a solution come back to us, we will figure something out (or ask in IRC).


I think you may be able to skip the lookup (which could get expensive) and instead rely on the fact that the user must have explicitly written that type as an elaborated type specifier when trying to calculate the source range for the return type. I suspect what's happening right now is that `findReturnTypeAndCVSourceRange()` isn't noticing the `struct` specifier and that's why it's getting dropped. If the user wrote it as an elaborated type specifier, we should probably retain that when shifting it to a trailing return type.



================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:132
+  // If the return type has no local qualifiers, it's source range is accurate.
+  if (!F.getReturnType().hasLocalQualifiers())
+    return ReturnTypeRange;
----------------
I think you may need to check here if the return type is an elaborated type (e.g., one that is written as `struct S` rather than `S`).


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:153
+        !ExtendedLeft) {
+      for (int J = static_cast<int>(I) - 1; J >= 0 && IsCV(Tokens[J]); J--)
+        ReturnTypeRange.setBegin(Tokens[J].getLocation());
----------------
And then look for the elaborated tag type here.


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

https://reviews.llvm.org/D56160