[PATCH] D133843: [clangd] Prefer definitions for gototype and implementation

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0a093f62d154: [clangd] Prefer definitions for gototype and 
implementation (authored by kadircet).

Changed prior to commit:
  https://reviews.llvm.org/D133843?vs=460003=542874#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133843

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp


Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1451,7 +1451,7 @@
   return Reply(Types.takeError());
 std::vector Response;
 for (const LocatedSymbol  : *Types)
-  Response.push_back(Sym.PreferredDeclaration);
+  
Response.push_back(Sym.Definition.value_or(Sym.PreferredDeclaration));
 return Reply(std::move(Response));
   });
 }
@@ -1467,7 +1467,7 @@
   return Reply(Overrides.takeError());
 std::vector Impls;
 for (const LocatedSymbol  : *Overrides)
-  Impls.push_back(Sym.PreferredDeclaration);
+  Impls.push_back(Sym.Definition.value_or(Sym.PreferredDeclaration));
 return Reply(std::move(Impls));
   });
 }


Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1451,7 +1451,7 @@
   return Reply(Types.takeError());
 std::vector Response;
 for (const LocatedSymbol  : *Types)
-  Response.push_back(Sym.PreferredDeclaration);
+  Response.push_back(Sym.Definition.value_or(Sym.PreferredDeclaration));
 return Reply(std::move(Response));
   });
 }
@@ -1467,7 +1467,7 @@
   return Reply(Overrides.takeError());
 std::vector Impls;
 for (const LocatedSymbol  : *Overrides)
-  Impls.push_back(Sym.PreferredDeclaration);
+  Impls.push_back(Sym.Definition.value_or(Sym.PreferredDeclaration));
 return Reply(std::move(Impls));
   });
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133843: [clangd] Prefer definitions for gototype and implementation

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Yeah, having used this feature for a while, you're right about all of this of 
course, the current behavior is infuriating.
Sorry!


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

[PATCH] D133843: [clangd] Prefer definitions for gototype and implementation

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet abandoned this revision.
kadircet added a comment.

abandoning in favor of D155898 


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

[PATCH] D133843: [clangd] Prefer definitions for gototype and implementation

[Tom Praschan via Phabricator via cfe-commits]

tom-anders added a comment.

> happy to add some comments for the reasoning if you (and possibly others 
> seeing this change) also agree that this is more useful

Can confirm that (at least to me) it's really annoying when it takes me to a 
forward declaration here. I 99% of the time will immediately trigger 
textDocument/definition when this happens.


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

[PATCH] D133843: [clangd] Prefer definitions for gototype and implementation

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In 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

[PATCH] D133843: [clangd] Prefer definitions for gototype and implementation

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

There's no test here, can you describe the cases you expect this to affect and 
why the new behavior is better?

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.

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!)


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

[PATCH] D133843: [clangd] Prefer definitions for gototype and implementation

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added reviewers: sammccall, usaxena95.
Herald added a subscriber: arphaman.
Herald added a project: All.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133843

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp


Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -615,7 +615,6 @@
  CodeAction::INFO_KIND}}}
   : llvm::json::Value(true);
 
-
   std::vector Commands;
   for (llvm::StringRef Command : Handlers.CommandHandlers.keys())
 Commands.push_back(Command);
@@ -1408,7 +1407,7 @@
   return Reply(Types.takeError());
 std::vector Response;
 for (const LocatedSymbol  : *Types)
-  Response.push_back(Sym.PreferredDeclaration);
+  
Response.push_back(Sym.Definition.value_or(Sym.PreferredDeclaration));
 return Reply(std::move(Response));
   });
 }
@@ -1424,7 +1423,7 @@
   return Reply(Overrides.takeError());
 std::vector Impls;
 for (const LocatedSymbol  : *Overrides)
-  Impls.push_back(Sym.PreferredDeclaration);
+  Impls.push_back(Sym.Definition.value_or(Sym.PreferredDeclaration));
 return Reply(std::move(Impls));
   });
 }


Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -615,7 +615,6 @@
  CodeAction::INFO_KIND}}}
   : llvm::json::Value(true);
 
-
   std::vector Commands;
   for (llvm::StringRef Command : Handlers.CommandHandlers.keys())
 Commands.push_back(Command);
@@ -1408,7 +1407,7 @@
   return Reply(Types.takeError());
 std::vector Response;
 for (const LocatedSymbol  : *Types)
-  Response.push_back(Sym.PreferredDeclaration);
+  Response.push_back(Sym.Definition.value_or(Sym.PreferredDeclaration));
 return Reply(std::move(Response));
   });
 }
@@ -1424,7 +1423,7 @@
   return Reply(Overrides.takeError());
 std::vector Impls;
 for (const LocatedSymbol  : *Overrides)
-  Impls.push_back(Sym.PreferredDeclaration);
+  Impls.push_back(Sym.Definition.value_or(Sym.PreferredDeclaration));
 return Reply(std::move(Impls));
   });
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits