Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to show parents in the initial type hierarchy tree as well #44

Merged
merged 3 commits into from
Oct 14, 2020

Conversation

HighCommander4
Copy link
Contributor

No description provided.

@HighCommander4
Copy link
Contributor Author

This required a server-side change as well: https://reviews.llvm.org/D81845

@HighCommander4
Copy link
Contributor Author

HighCommander4 commented Jun 29, 2020

(This should be ready to review along with the dependent PR #20.)

@HighCommander4
Copy link
Contributor Author

(Rebased on top of the landing of #20.)

@hokein
Copy link
Collaborator

hokein commented Aug 5, 2020

I played it around locally -- it works great for simple examples! but for a real LLVM project, I encountered some issues (some of them might be related to clangd), below are the details

  • open clang/lib/ARCMigrate/TransUnbridgedCasts.cpp, invoke type hierarchy on UnbridgedCastRewriter (Line 59), the debug console throws an exception [1];
  • results on RecursiveASTVisitor[2] seem odd, and incomplete;
  • no result on clang::clangd::ParsingCallbacks, it maybe caused by the fact that we don't collect main-file symbols;
rejected promise not handled within 1 second: Error: Cannot resolve tree item for element 0/0:RecursiveASTVisitor<(anonymous namespace)::UnbridgedCastRewriter>/0:UnbridgedCastRewriter
extensionHostProcess.js:976
stack trace: Error: Cannot resolve tree item for element 0/0:RecursiveASTVisitor<(anonymous namespace)::UnbridgedCastRewriter>/0:UnbridgedCastRewriter
	at /Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/services/extensions/node/extensionHostProcess.js:609:64

[2]:

image

@HighCommander4
Copy link
Contributor Author

I played it around locally -- it works great for simple examples! but for a real LLVM project, I encountered some issues (some of them might be related to clangd), below are the details

Thanks very much for testing!

  • open clang/lib/ARCMigrate/TransUnbridgedCasts.cpp, invoke type hierarchy on UnbridgedCastRewriter (Line 59), the debug console throws an exception [1];

I'm able to reproduce this, somewhat intermittently. I don't really understand the cause; as far as I can tell, I'm using the VSCode API as documented (e.g. implementing TreeDataProvider.getParent() so I can use TreeView.reveal()). Also, I don't see any actual problem caused by it (i.e. the tree shows up just like in the cases where I don't get the error).

I added a commit to the PR which provides a rejection handler to catch this error and print a warning instead of letting VSCode throw an exception; I don't have a better idea for what to do.

  • results on RecursiveASTVisitor[2] seem odd, and incomplete;

There turned out to be three different server-side issues here: clangd/clangd#504, clangd/clangd#507, and clangd/clangd#510. The first two are fixed and I have a patch up for the third.

  • no result on clang::clangd::ParsingCallbacks, it maybe caused by the fact that we don't collect main-file symbols;

This is also fixed by clangd/clangd#510.

@hokein
Copy link
Collaborator

hokein commented Oct 1, 2020

@HighCommander4 could you rebase it to master?

@HighCommander4
Copy link
Contributor Author

Rebased.

@HighCommander4
Copy link
Contributor Author

Rebased again.

@HighCommander4
Copy link
Contributor Author

@hokein do you think this is ready to be merged now? The issues described in this comment are now all resolved, and the patch has been rebased.

Copy link
Collaborator

@hokein hokein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

// Sometimes TreeView.reveal() fails. It's unclear why, and it does
// not appear to have any visible effects, but vscode complains if
// you don't handle the rejection promise, so we do so and log a
// warning.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is very unfortunate, but it seems there are no visible effects, we can live with that.

btw, I see some logs (not sure they are helpful)

Warning: TreeView.reveal() failed for reason: Error: Cannot resolve tree item for element 0/0:RecursiveASTVisitor<(anonymous namespace)::UnbridgedCastRewriter>/0:UnbridgedCastRewriter
type-hierarchy.js:250
Warning: TreeView.reveal() failed for reason: Error: TreeError [clangd.typeHierarchyView] Data tree node not found: [object Object]

@hokein hokein merged commit bda2842 into clangd:master Oct 14, 2020
@HighCommander4
Copy link
Contributor Author

Thanks!

btw, I see some logs (not sure they are helpful)

I see the same thing. Unfortunately, I have not been able to figure out what sort of misuse of the TreeView API (if any) this translates to. If anyone has ideas about this, I'm happy to come back to this and try them out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants