Page MenuHomePhabricator

nridge (Nathan Ridge)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 4 2018, 8:41 PM (58 w, 6 d)

Recent Activity

Fri, Mar 22

nridge updated the diff for D59407: [clangd] Add RelationSlab.

Scrapped 'struct Relation' and addressed other comments

Fri, Mar 22, 8:28 AM · Restricted Project
nridge added inline comments to D59407: [clangd] Add RelationSlab.
Fri, Mar 22, 8:05 AM · Restricted Project

Thu, Mar 21

nridge added inline comments to D59407: [clangd] Add RelationSlab.
Thu, Mar 21, 7:01 PM · Restricted Project

Sun, Mar 17

nridge updated the diff for D59407: [clangd] Add RelationSlab.

Address review comments, except for the deduplication which is still under discussion

Sun, Mar 17, 12:21 PM · Restricted Project
nridge added inline comments to D59407: [clangd] Add RelationSlab.
Sun, Mar 17, 12:21 PM · Restricted Project
nridge added a comment to D59407: [clangd] Add RelationSlab.

I believe it makes sense to deduplicate SymbolIDs for RelationSlab.
Up until now, we mostly had only one occurence of a SymbolID in a Slab, but RelationSlab does not follow that assumption.

Sun, Mar 17, 11:10 AM · Restricted Project
nridge added a comment to D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes.

Also please have a look at D59083, and make sure it helps implement what you have in might and let me know if there's anything missing.

Sun, Mar 17, 10:52 AM · Restricted Project
nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

Great, thanks for the reviews!

Sun, Mar 17, 10:52 AM · Restricted Project, Restricted Project

Thu, Mar 14

nridge added a comment to D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes.

Hi Nathan,

I would also suggest splitting up current changes so that we can start reviewing them, which might result in other changes in your planned changes and help reduce duplicate work both on our and your side.
[...]

As for splitting changes I would suggest something like:

  • RelationSlab
  • Serialization/Deserialization of the RelationSlab
  • Integrating RelationSlab with SymbolCollector
  • Adding queries to index structures(memindex, dex, mergedindex etc.)
  • Surfacing those queries through typehierarchy request
Thu, Mar 14, 10:31 PM · Restricted Project
nridge added inline comments to D59407: [clangd] Add RelationSlab.
Thu, Mar 14, 10:31 PM · Restricted Project
nridge created D59407: [clangd] Add RelationSlab.
Thu, Mar 14, 10:28 PM · Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Address latest review comments

Thu, Mar 14, 4:57 PM · Restricted Project, Restricted Project
nridge added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Thu, Mar 14, 4:55 PM · Restricted Project, Restricted Project

Tue, Mar 12

nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Address remaining review comments

Tue, Mar 12, 10:25 PM · Restricted Project, Restricted Project
nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

Unfortunately we usually test LSP layer with lit tests, and since we don't have any lit tests for this use-case it wasn't caught. Could you add a lit test for overall use case? You can see examples inside clang-tools-extra/test/clangd/ folder.

Tue, Mar 12, 7:07 PM · Restricted Project, Restricted Project
nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

Unfortunately, there is a further problem: the Theia client-side implementation of type hierarchy has recently merged, and their code has changed so that they do require typeHierarchy/resolve to be supported. They ask for 1 level in the initial request, ignore any extra levels the server might send, and ask for extra levels using typeHierarchy/resolve instead.

What should we do here -- seek to change Theia, or implement typeHierachy/resolve for supertypes after all?

Looking at https://github.com/theia-ide/theia/pull/3802#issuecomment-455992523 inside theia's implementation of this feature, I believe these are all subject to change.

Tue, Mar 12, 7:05 PM · Restricted Project, Restricted Project

Mon, Mar 11

nridge added a comment to D59083: [clangd] Store explicit template specializations in index for code navigation purposes.

One of the use cases I imagined for this (unrelated to D58880) is that if the user searches for vector (using workspace/symbols), they get a separate search result for the vector primary template, and a separate one for vector<bool>. For this to be useful, the server needs to print the <bool> as part of the search result.

Mon, Mar 11, 7:37 AM · Restricted Project, Restricted Project
nridge added a comment to D59083: [clangd] Store explicit template specializations in index for code navigation purposes.

Is any representation of the template arguments stored in the index?

Mon, Mar 11, 7:12 AM · Restricted Project, Restricted Project

Sun, Mar 10

nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

Unfortunately, there is a further problem: the Theia client-side implementation of type hierarchy has recently merged, and their code has changed so that they do require typeHierarchy/resolve to be supported. They ask for 1 level in the initial request, ignore any extra levels the server might send, and ask for extra levels using typeHierarchy/resolve instead.

Sun, Mar 10, 8:24 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Fix a (somewhat amusing) typo where I wrote '0' instead of 'O' in a fromJSON() implementation

Sun, Mar 10, 8:20 PM · Restricted Project, Restricted Project
nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

The updated patch addresses the infinite recursion issue by bailing on dependent bases for now, as Sam suggested. I will implement the more comprehensive suggested fix ("bail out once we see instantiations of the same template decl twice in a parent chain") in a follow-up patch. I did add all three testcases that have come up during discussion.

Sun, Mar 10, 7:50 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Address the infinite recursion issue

Sun, Mar 10, 7:42 PM · Restricted Project, Restricted Project

Thu, Mar 7

nridge planned changes to D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes.

Thanks for the detailed analysis!

Thu, Mar 7, 6:02 PM · Restricted Project

Tue, Mar 5

nridge added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Tue, Mar 5, 7:56 PM · Restricted Project, Restricted Project
nridge added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Tue, Mar 5, 7:55 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Address most of the latest review comments.

Tue, Mar 5, 7:55 PM · Restricted Project, Restricted Project
nridge added a comment to D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes.

I think we can extend the existing Ref to support it, so that most of the stuff could be reused, rather than implementing a new slab:

  • introduce a new RefKind, like BaseOf
  • add a new field SymbolID in Ref
Tue, Mar 5, 11:13 AM · Restricted Project

Mon, Mar 4

nridge added a comment to D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes.

Sorry I didn't notice the mailing thread beforehand, looks like Sam had a good point regarding performing operations on types rather than symbols(http://lists.llvm.org/pipermail/clangd-dev/2019-January/000241.html).
Does current implementation take this into account by either pointing at template specializations and/or base template declarations?

Mon, Mar 4, 9:58 PM · Restricted Project
nridge updated the diff for D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes.

Add tests involving templates and template specializations

Mon, Mar 4, 9:52 PM · Restricted Project

Sun, Mar 3

nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Address a couple of outstanding TODOs

Sun, Mar 3, 8:58 PM · Restricted Project, Restricted Project
nridge removed a reviewer for D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes: sammccall.

Removing Sam as reviewer as he's on parental leave.

Sun, Mar 3, 11:44 AM · Restricted Project
nridge updated the diff for D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes.

Rebased onto updated D56370

Sun, Mar 3, 11:44 AM · Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Address latest review comments

Sun, Mar 3, 11:30 AM · Restricted Project, Restricted Project
nridge added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Sun, Mar 3, 11:27 AM · Restricted Project, Restricted Project

Sat, Mar 2

nridge added a parent revision for D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes: D56370: [clangd] Add support for type hierarchy (super types only for now).
Sat, Mar 2, 5:23 PM · Restricted Project
nridge added a child revision for D56370: [clangd] Add support for type hierarchy (super types only for now): D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes.
Sat, Mar 2, 5:23 PM · Restricted Project, Restricted Project
nridge created D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes.
Sat, Mar 2, 5:23 PM · Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Rebased.

Sat, Mar 2, 5:17 PM · Restricted Project, Restricted Project

Tue, Feb 26

nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

I think this is ready to continue to be reviewed :)

Tue, Feb 26, 4:04 PM · Restricted Project, Restricted Project

Feb 14 2019

nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Fix a test failure

Feb 14 2019, 4:49 PM · Restricted Project, Restricted Project
nridge added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Feb 14 2019, 4:30 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).
  • Rework tests to test the lower-level functions like typeAncestors()
  • Remove support for typeHierarchy/resolve
Feb 14 2019, 4:29 PM · Restricted Project, Restricted Project

Feb 12 2019

nridge added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Feb 12 2019, 8:38 PM · Restricted Project, Restricted Project
nridge added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Feb 12 2019, 6:22 PM · Restricted Project, Restricted Project

Feb 9 2019

nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).
  • in 'XRefs.h', I think the API to introduce is findTypeAt(Position) -> Decl* + typeAncestors(Decl*) and later typeDescendants(Decl*), rather than a single more complex typeHierarchy call. These two operations have little in common implementation-wise, and it's easy to imagine editors preferring to expose them separately. In clangdserver of course we need to expose a single operation because of transactionality. The stitching together could go in clangdserver, or a higher-level function exposed by xrefs - but I think the separate functions are what we should be unit-testing.
Feb 9 2019, 5:29 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Introduce and use findRecordTypeAt() and typeAncestors() helpers

Feb 9 2019, 5:15 PM · Restricted Project, Restricted Project
nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

So on a concrete but still high-level:

  • I don't think we should implement the resolve operation, or resolution bounds, at this point - this patch doesn't do anything slow. Returning complete ancestors and never returning any children seems simplest.
Feb 9 2019, 4:25 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Address Kadir's review comments

Feb 9 2019, 4:21 PM · Restricted Project, Restricted Project
nridge planned changes to D56370: [clangd] Add support for type hierarchy (super types only for now).
Feb 9 2019, 4:21 PM · Restricted Project, Restricted Project
nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

Thank you for the reviews!

Feb 9 2019, 4:21 PM · Restricted Project, Restricted Project
nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

Add tests for scenarios involving class template specializations

Also improve support for dependent bases

Feb 9 2019, 3:23 PM · Restricted Project, Restricted Project
nridge planned changes to D56370: [clangd] Add support for type hierarchy (super types only for now).
Feb 9 2019, 3:19 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Add tests for scenarios involving class template specializations

Feb 9 2019, 3:19 PM · Restricted Project, Restricted Project

Feb 3 2019

nridge added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Feb 3 2019, 4:06 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

remove unrelated file

Feb 3 2019, 4:04 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

This completes the implementation of the revised spec (for supertypes only)

Feb 3 2019, 4:03 PM · Restricted Project, Restricted Project

Jan 31 2019

nridge added a comment to D56611: [clangd] A code action to swap branches of an if statement.

Fix here: https://reviews.llvm.org/D57560

Jan 31 2019, 6:01 PM · Restricted Project, Restricted Project
nridge created D57560: [clangd] Link libclangAST into clangd tool.
Jan 31 2019, 6:01 PM · Restricted Project, Restricted Project
nridge added a comment to D56611: [clangd] A code action to swap branches of an if statement.

The complete commands that's failing is:

Jan 31 2019, 5:55 PM · Restricted Project, Restricted Project
nridge added a comment to D56611: [clangd] A code action to swap branches of an if statement.

Hi Nathan,
What platform is this on? Not seeing it on the buildbots.
Anything unusual in build setup (standalone build, building with shared libraries, etc)?

Jan 31 2019, 5:52 PM · Restricted Project, Restricted Project
Herald added a project to D56611: [clangd] A code action to swap branches of an if statement: Restricted Project.

This commit is causing clangd to fail to link, with errors like this:

Jan 31 2019, 4:39 PM · Restricted Project, Restricted Project

Jan 29 2019

nridge added a comment to D56653: [clangd] Penalize file-scope symbols in the ranking for non-completion queries.

Friendly reminder than this is ready to be committed :)

Jan 29 2019, 6:55 PM · Restricted Project

Jan 27 2019

nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

This is a partial implementation of the revised spec for type hierarchy. Things that still need to be done:

Jan 27 2019, 8:08 PM · Restricted Project, Restricted Project
nridge retitled D56370: [clangd] Add support for type hierarchy (super types only for now) from [clangd] Add support for the textDocument/superTypes request to [clangd] Add support for type hierarchy (super types only for now).
Jan 27 2019, 8:05 PM · Restricted Project, Restricted Project
nridge planned changes to D56370: [clangd] Add support for type hierarchy (super types only for now).
Jan 27 2019, 8:04 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Remove unrelated file

Jan 27 2019, 8:04 PM · Restricted Project, Restricted Project
nridge planned changes to D56370: [clangd] Add support for type hierarchy (super types only for now).
Jan 27 2019, 8:04 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Implement revised specification (WIP)

Jan 27 2019, 8:03 PM · Restricted Project, Restricted Project

Jan 25 2019

nridge planned changes to D56370: [clangd] Add support for type hierarchy (super types only for now).

I am just wondering which data structure fits the best here. Do you have any thoughts on support for virtual inheritance? In other words - should we use a tree or a DAG?

Jan 25 2019, 6:20 PM · Restricted Project, Restricted Project

Jan 24 2019

nridge added a comment to D55739: [Clangd] Index main-file macros (bug 39761).

Just realize one missing point: we should respect the CollectMainFileSymbols option, otherwise looks good.

Jan 24 2019, 8:08 AM
nridge updated the diff for D55739: [Clangd] Index main-file macros (bug 39761).

Don't include unrelated file

Jan 24 2019, 8:06 AM
nridge updated the diff for D55739: [Clangd] Index main-file macros (bug 39761).

Address more review comments

Jan 24 2019, 8:05 AM

Jan 22 2019

nridge added inline comments to D55739: [Clangd] Index main-file macros (bug 39761).
Jan 22 2019, 7:10 PM
nridge updated the diff for D55739: [Clangd] Index main-file macros (bug 39761).

Address review comments

Jan 22 2019, 7:08 PM
nridge added a comment to D57077: [clangd] Link clangTidy into clangd tests.

After pulling the latest code, I found this change was necessary to get past the following linker error:

Jan 22 2019, 6:52 PM
nridge created D57077: [clangd] Link clangTidy into clangd tests.
Jan 22 2019, 6:52 PM

Jan 19 2019

nridge added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

Please see also the clangd-dev discussion about this feature.

Jan 19 2019, 2:14 PM · Restricted Project, Restricted Project
nridge updated the summary of D56370: [clangd] Add support for type hierarchy (super types only for now).
Jan 19 2019, 2:09 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Cleaned up patch, ready for review

Jan 19 2019, 2:08 PM · Restricted Project, Restricted Project

Jan 18 2019

nridge added reviewers for D55739: [Clangd] Index main-file macros (bug 39761): sammccall, hokein.

@hokein, @sammccall, this is a follow-up to https://reviews.llvm.org/D55185. Would one (or both) of you be intereested in reviewing it?

Jan 18 2019, 11:10 AM
nridge added a comment to D56653: [clangd] Penalize file-scope symbols in the ranking for non-completion queries.

@sammccall, could you commit this patch for me? Thank you!

Jan 18 2019, 11:10 AM · Restricted Project

Jan 15 2019

nridge updated the diff for D55739: [Clangd] Index main-file macros (bug 39761).

Rebase

Jan 15 2019, 6:02 PM
nridge updated the diff for D56653: [clangd] Penalize file-scope symbols in the ranking for non-completion queries.

Address review comments

Jan 15 2019, 4:12 PM · Restricted Project

Jan 13 2019

nridge added a comment to D55185: [Clangd] Index main-file symbols (bug 39761).
  • set Scope accordingly in Quality.cpp, and adjust relevance scores for non-code-complete queries based on scope, in the opposide direction from CC queries (I'm happy to do this if you don't want to)

I'm happy to give this a try. I think this one makes more sense in a separate patch.

Jan 13 2019, 3:40 PM
nridge created D56653: [clangd] Penalize file-scope symbols in the ranking for non-completion queries.
Jan 13 2019, 3:38 PM · Restricted Project

Jan 12 2019

nridge added a comment to D55185: [Clangd] Index main-file symbols (bug 39761).

SymbolIndex::fuzzyFind provides ranking for non-completion searches.
This is used for workspaceSymbol at the moment and maybe other queries (the subtypes search we discussed on the mailing list?) in future.

Currently the scope isn't taken into account for non-completion queries, because historically only top-level global symbols were in the index. With members and now main-file symbols in the index, we should fix that.

Jan 12 2019, 8:21 PM
nridge updated the diff for D55185: [Clangd] Index main-file symbols (bug 39761).

Add a new symbol flag to mark main-file symbols

Jan 12 2019, 8:17 PM

Jan 10 2019

nridge updated the diff for D55185: [Clangd] Index main-file symbols (bug 39761).

Addressed Sam's comments, except for the SymbolFlag one as I do not understand it yet

Jan 10 2019, 12:34 PM
nridge added a comment to D55185: [Clangd] Index main-file symbols (bug 39761).

The piece that seems to be missing is recording that a symbol is main-file only, which seems important for ranking - public symbols should be ranked above private ones.
(In Quality.h this is the distinction between AccessibleScope::GlobalScope and AccessibleScope::FileScope).

Jan 10 2019, 12:24 PM

Jan 9 2019

nridge added a comment to D55185: [Clangd] Index main-file symbols (bug 39761).

looks good. Do you have commit access?

Jan 9 2019, 7:30 AM

Jan 8 2019

nridge updated the diff for D55185: [Clangd] Index main-file symbols (bug 39761).

Address latest review comment

Jan 8 2019, 3:22 PM

Jan 6 2019

nridge retitled D56370: [clangd] Add support for type hierarchy (super types only for now) from Add support for the textDocument/superTypes request to [clangd] Add support for the textDocument/superTypes request.
Jan 6 2019, 4:43 PM · Restricted Project, Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).
Jan 6 2019, 4:42 PM · Restricted Project, Restricted Project
nridge planned changes to D56370: [clangd] Add support for type hierarchy (super types only for now).
Jan 6 2019, 4:42 PM · Restricted Project, Restricted Project
nridge created D56370: [clangd] Add support for type hierarchy (super types only for now).
Jan 6 2019, 4:32 PM · Restricted Project, Restricted Project
nridge planned changes to D56370: [clangd] Add support for type hierarchy (super types only for now).
Jan 6 2019, 4:32 PM · Restricted Project, Restricted Project

Jan 5 2019

nridge added a comment to D56314: [clangd] Don't store completion info if the symbol is not used for code completion..

Might we want to keep some of this information for workspace/symbol? I mean, surely not "documentation", but perhaps "signature" and "return type"?

Jan 5 2019, 7:38 PM
nridge updated the diff for D55185: [Clangd] Index main-file symbols (bug 39761).

Addressed latest round of review comments

Jan 5 2019, 5:51 PM
nridge added inline comments to D55185: [Clangd] Index main-file symbols (bug 39761).
Jan 5 2019, 5:51 PM

Jan 3 2019

nridge added a comment to D55185: [Clangd] Index main-file symbols (bug 39761).

I did double check today, I can confirmed that both of them ran at the same code base.

Jan 3 2019, 5:05 PM