kadircet added a comment. In D133843#3791127 <https://reviews.llvm.org/D133843#3791127>, @sammccall wrote:
> There's no test here, can you describe the cases you expect this to affect > and why the new behavior is better? right, sorry for the obscure patch. giving examples/details in particular cases below. > For types this seems doubly-dubious at first glance: > > - when we've decided to "prefer" a non-definition declaration, why make an > exception here? > - I'd expect the definition to almost-always be the preferred declaration > anyway. So apart from the special cases this happens, at the moment we have a discrepancy between go-to-def on a type and go-to-type on a decl, which to me seems like a surprise and definitely unexpected. At least to me the main point of go-to-type interaction is to get rid of the extra jump, and when it takes me to declaration for whatever reason, that benefit is lost. As for the particular cases I've noticed this happening, it's forward declarations visible from the definition of a type, which manifests in a couple different forms: - A header depending on the type forward declaring it, and also being included by the TU defining the type (sort of a circular dependency, so might not be as important). - A type that's being forward declared by the same header defining it, while the type still being public (which I think is common when there are interactions between two different types). - A private type that's being forward declared in a header and defined in a private header or implementation file. To me it seems like only the last case **might** warrant a jump to declaration, but I think it's still to prefer definition in that case. In other cases we most definitely want the definition directly. > For implementation, I think I there's a good argument for this behavior: the > purpose of the command is to cut through abstractions to see what this method > does in practice, and finding the definition fits with that. I'm happy with > this behavior but I think it deserves a comment. > (There's also a bad argument: users think "implementation" means definition - > just because the name is ambiguous doesn't mean we should implement both > behaviors!) Right, this was my motivation as well. Especially in UI-rich editors like VSCode, one actually gets snippets around the locations and seeing snippets around the definition definitely feels more helpful. happy to add some comments for the reasoning if you (and possibly others seeing this change) also agree that this is more useful Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133843/new/ https://reviews.llvm.org/D133843 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
