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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
