[PATCH] D70357: [clangd] Untangle Hover from XRefs, move into own file.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/Hover.cpp:374
+  } else {
+auto Offset = positionToOffset(SM.getBufferData(SM.getMainFileID()), Pos);
+if (!Offset) {

sammccall wrote:
> kadircet wrote:
> > kadircet wrote:
> > > nit: `SM.getFileOffset(SourceLocationBeg)` ?
> > why not just expose getDeclAtPosition in `AST.h` ?
> > nit: SM.getFileOffset(SourceLocationBeg) ?
> I was deliberately trying to avoid using the "rewind token" logic where it's 
> not needed. We've had bugs with it before, as well as with selectiontree, and 
> composing them unneccesarily is harder to debug.
> 
> > why not just expose getDeclAtPosition in AST.h ?
> I don't think it's better than the expanded form - it's just plugging a 
> couple of functions together, and the choices it makes (flattening 
> SourceLocation into an offset, the DeclRelationSet, only looking at the 
> common ancestor and nothing higher up) aren't obvious.
> 
> It also privileges decls over other types of things (e.g. the followup patch 
> looks for Exprs in the selection tree to show their values, and that couldn't 
> happen if it was hidden behind getDeclAtPosition)
> I was deliberately trying to avoid using the "rewind token" logic where it's 
> not needed. We've had bugs with it before, as well as with selectiontree, and 
> composing them unneccesarily is harder to debug.

Sounds good, I was worried about performing this conversion twice, but I 
suppose it should be OK for hover's performance.



Comment at: clang-tools-extra/clangd/Hover.cpp:380
+SelectionTree Selection(AST.getASTContext(), AST.getTokens(), *Offset);
+std::vector Result;
+if (const SelectionTree::Node *N = Selection.commonAncestor()) {

nit: Unused


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70357: [clangd] Untangle Hover from XRefs, move into own file.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 4 inline comments as done.
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:374
+  } else {
+auto Offset = positionToOffset(SM.getBufferData(SM.getMainFileID()), Pos);
+if (!Offset) {

kadircet wrote:
> kadircet wrote:
> > nit: `SM.getFileOffset(SourceLocationBeg)` ?
> why not just expose getDeclAtPosition in `AST.h` ?
> nit: SM.getFileOffset(SourceLocationBeg) ?
I was deliberately trying to avoid using the "rewind token" logic where it's 
not needed. We've had bugs with it before, as well as with selectiontree, and 
composing them unneccesarily is harder to debug.

> why not just expose getDeclAtPosition in AST.h ?
I don't think it's better than the expanded form - it's just plugging a couple 
of functions together, and the choices it makes (flattening SourceLocation into 
an offset, the DeclRelationSet, only looking at the common ancestor and nothing 
higher up) aren't obvious.

It also privileges decls over other types of things (e.g. the followup patch 
looks for Exprs in the selection tree to show their values, and that couldn't 
happen if it was hidden behind getDeclAtPosition)



Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:13
+#include "TestTU.h"
+
+#include "clang/Basic/SourceManager.h"

kadircet wrote:
> unintended newline ?
intended (subproject vs clang) but dosen't seem to match local style. removed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70357: [clangd] Untangle Hover from XRefs, move into own file.

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel added a comment.

I had a look at the build problems:
https://github.com/google/llvm-premerge-checks/issues/56


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70357: [clangd] Untangle Hover from XRefs, move into own file.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:374
+  } else {
+auto Offset = positionToOffset(SM.getBufferData(SM.getMainFileID()), Pos);
+if (!Offset) {

nit: `SM.getFileOffset(SourceLocationBeg)` ?



Comment at: clang-tools-extra/clangd/Hover.cpp:374
+  } else {
+auto Offset = positionToOffset(SM.getBufferData(SM.getMainFileID()), Pos);
+if (!Offset) {

kadircet wrote:
> nit: `SM.getFileOffset(SourceLocationBeg)` ?
why not just expose getDeclAtPosition in `AST.h` ?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:80
   // if we can't resolve the type, return an error message
-  if (DeducedType == llvm::None || DeducedType->isNull()) {
+  if (DeducedType == llvm::None) {
 return createErrorMessage("Could not deduce type for 'auto' type", Inputs);

nit: while there, maybe get rid of braces as well?



Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:13
+#include "TestTU.h"
+
+#include "clang/Basic/SourceManager.h"

unintended newline ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70357: [clangd] Untangle Hover from XRefs, move into own file.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: kuhnel.
sammccall added a comment.

@kuhnel: I think the premerge guards bot is acting up here: the first snapshot 
was definitely broken (missing files).
And even the second the ninja logs don't show building the new files. I think 
it might just be building the tree without patching? The console log shows 
errors around the arc patch step, and suspiciously few details about the patch 
being applied.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70357: [clangd] Untangle Hover from XRefs, move into own file.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 60143 tests passed, 0 failed and 729 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70357: [clangd] Untangle Hover from XRefs, move into own file.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 60143 tests passed, 0 failed and 729 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits