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

[Bernhard Manfred Gruber via Phabricator via cfe-commits]

bernhardmgruber marked 2 inline comments as done.
bernhardmgruber added a comment.

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.
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. Also, when my check rewrites `struct Object m();`, it correctly produces `auto m() -> struct Object;` (`findReturnTypeAndCVSourceRange()` includes the `struct`).

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.

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


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

https://reviews.llvm.org/D56160