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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D56160#1372766 <https://reviews.llvm.org/D56160#1372766>, @bernhardmgruber wrote:

> Thank you for the feedback!
>
> @JonasToth I added tests for `decltype` and i can rewrite all signatures where `decltype` is not the top level expression. The reason is, that the source range of a `clang::DecltypeType` is not accurate, as it does not include the expression within the parenthesis following the `decltype` keyword. There is even a FIXME somewhere in the corresponding source file.
>
> @aaron.ballman I am not sure what you mean and maybe you have not understood my issue correctly.
>
> In D56160#1369986 <https://reviews.llvm.org/D56160#1369986>, @aaron.ballman wrote:
>
> > > ...
> >
> > 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.
>
>
> Given the following source code before the rewriting:
>
>   struct Object { long long value; };
>   class C {
>     int Object;
>     struct Object m();
>   };
>   Object C::m() { return {0}; }
>
>
> The member function `struct Object m();` needs to have a `struct` before `Object`, because otherwise, the return type would conflict with the member variable of the same name.
>  On the contrary, the out-of-line definition `Object C::m() { return {0}; }` does not need the `struct`, as the scope of the return type does not include the member variables of class `C`. However, rewriting the out-of-line definition from `Object C::m() { return {0}; }` to `auto C::m() -> Object { return {0}; }` changes the scope of the return type, which now includes the member variable `int Object;` and results in a compilation error.


Understood. I was saying that if the inline member declaration uses an elaborated type specifier, the out-of-line, trailing-return-type variant needs to do so as well.

> I do not understand what you meant by
> 
>> the user must have explicitly written that type as an elaborated type specifier
> 
> because in case of the out-of-line defintion, the user is not required to do so.

But in the *declaration*, they had to, right?

> Also, when my check rewrites `struct Object m();`, it correctly produces `auto m() -> struct Object;` (`findReturnTypeAndCVSourceRange()` includes the `struct`).

Ah, that's good to know. I was taking a stab as to why it was misbehaving and it seems I stabbed wrong. :-P

> I tried to circumvent the problem doing something like (F is the matched `FunctionDecl`)
> 
>   if (auto M = dyn_cast<CXXMethodDecl>(F)) {
>     if (auto Name = M->getDeclaredReturnType().getBaseTypeIdentifier()) {
>       auto result = M->getDeclContext()->lookup(Name);
>       if (!result.empty()) {
>         // cannot do rewrite, collision
>       }
>     }
>   }
>    
> 
> but then I noticed, the lookup is only performed in the scope of the member function's class, not including e.g. inherited classes. So if we extend the example with
> 
>   class D : public C {
>       ::Object g();
>   };
>   Object D::g() {
> 
> 
> (note that instead of `struct Object` we can also qualify the type with the namespace it comes from), then the `DeclContext` of the out-of-line definition of the member function `g` is empty (it least, I do not get an output when i call `dumpDeclContext`). So maybe the `DeclContext` is not the right tool for the job. How else can I query all visible names in which a given object (in this case the matched (member) function) is declared? So I can check that the name of the return type is not already taken in the scope of the trailing return type.
> 
> Here is btw. the function case:
> 
>   struct Object { long long value; };
>   Object j1(unsigned Object) { return {Object * 2}; }
> 
> 
> After the rewrite, the return type conflicts with the parameter name.

Ah, that's a very good point. :-/

> I appreciate any input! I will continue to explore the problem. Maybe I can get it working by inspecting a multitude of `DeclContext`s.

I suspect the lookup operations you'd need to implement may not even be feasible from within clang-tidy. Sema is what handles the lookup logic (see SemaLookup.cpp) and you do not have access to that from within clang-tidy. It may be that we document this as a best-effort and add test cases (and update documentation) to demonstrate the problematic corner cases with FIXME comments.


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

https://reviews.llvm.org/D56160