Page MenuHomePhabricator

nridge (Nathan Ridge)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 4 2018, 8:41 PM (67 w, 19 h)

Recent Activity

Today

nridge added a comment to D60953: [clangd] Respect clang-tidy suppression comments.

Thank you MaskRay!

Mon, May 20, 2:54 PM · Restricted Project, Restricted Project

Fri, May 17

nridge added a comment to D60953: [clangd] Respect clang-tidy suppression comments.

Could someone merge this and D61841 now that they're approved? Thanks!

Fri, May 17, 8:08 AM · Restricted Project, Restricted Project

Thu, May 16

nridge edited reviewers for D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy, added: sammccall; removed: ilya-biryukov.

Changing reviewer to @sammccall as the updates to the patch are closely related to the discussion in D60953.

Thu, May 16, 6:50 PM · Restricted Project, Restricted Project
nridge updated the diff for D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy.

Update as per changes to dependent patch D60953

Thu, May 16, 6:50 PM · Restricted Project, Restricted Project
nridge updated the diff for D60953: [clangd] Respect clang-tidy suppression comments.

Address latest review comments

Thu, May 16, 6:47 PM · Restricted Project, Restricted Project

Tue, May 14

nridge added inline comments to D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy.
Tue, May 14, 9:28 PM · Restricted Project, Restricted Project
nridge updated the diff for D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy.

Address review comment

Tue, May 14, 9:25 PM · Restricted Project, Restricted Project
nridge added inline comments to D60953: [clangd] Respect clang-tidy suppression comments.
Tue, May 14, 9:25 PM · Restricted Project, Restricted Project
nridge updated the diff for D60953: [clangd] Respect clang-tidy suppression comments.

Address review comments, except for the derived class one, about which I have a question

Tue, May 14, 9:23 PM · Restricted Project, Restricted Project

Sun, May 12

nridge planned changes to D61842: [clangd] [WIP] [Not ready for review] Semantic highlighting.
Sun, May 12, 8:59 PM · Restricted Project
nridge retitled D61842: [clangd] [WIP] [Not ready for review] Semantic highlighting from [WIP] [Not ready for review] Semantic highlighting to [clangd] [WIP] [Not ready for review] Semantic highlighting.
Sun, May 12, 8:59 PM · Restricted Project
nridge updated the diff for D61842: [clangd] [WIP] [Not ready for review] Semantic highlighting.

Add [clangd] to title

Sun, May 12, 8:59 PM · Restricted Project
nridge planned changes to D61842: [clangd] [WIP] [Not ready for review] Semantic highlighting.
Sun, May 12, 8:53 PM · Restricted Project
nridge created D61842: [clangd] [WIP] [Not ready for review] Semantic highlighting.
Sun, May 12, 8:53 PM · Restricted Project
nridge added a comment to D60953: [clangd] Respect clang-tidy suppression comments.

Friendly review ping.

Sun, May 12, 8:52 PM · Restricted Project, Restricted Project
nridge created D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy.
Sun, May 12, 8:52 PM · Restricted Project, Restricted Project

Wed, May 8

Herald added a project to D41911: [clangd] Include scanner that finds compile commands for headers.: Restricted Project.

Are there are plans to move forward with this sort of an include scanning approach?

Wed, May 8, 7:46 PM · Restricted Project

Sun, Apr 28

nridge added inline comments to D60953: [clangd] Respect clang-tidy suppression comments.
Sun, Apr 28, 3:07 PM · Restricted Project, Restricted Project
nridge updated the diff for D60953: [clangd] Respect clang-tidy suppression comments.

Address review comments

Sun, Apr 28, 3:06 PM · Restricted Project, Restricted Project

Thu, Apr 25

nridge edited reviewers for D60953: [clangd] Respect clang-tidy suppression comments, added: ilya-biryukov; removed: hokein.

Changing reviewers as I understand @hokein is on vacation.

Thu, Apr 25, 9:58 PM · Restricted Project, Restricted Project
nridge planned changes to D59407: [clangd] Add RelationSlab.

So if you can stomach it, I think

Approach 2: Add a RelationSlab storing (subject, predicate, object) triples, intended for sparse relations*

is certainly fine with us (having spoken with @kadircet @ilya-biryukov @sammccall @gribozavr - @hokein is on vacation but not nearly as stubborn as I am!)

Thu, Apr 25, 9:57 PM · Restricted Project

Sun, Apr 21

nridge added a comment to D59756: [clangd] Support dependent bases in type hierarchy.

Thanks!

Sun, Apr 21, 7:00 PM · Restricted Project, Restricted Project
nridge added a comment to D60954: [clangd] Test case demonstrating variable template issue.

Please see further details in this clangd-dev post.

Sun, Apr 21, 2:42 PM · Restricted Project
nridge created D60954: [clangd] Test case demonstrating variable template issue.
Sun, Apr 21, 2:14 PM · Restricted Project
nridge added a comment to D59407: [clangd] Add RelationSlab.

Hi, any update here? I would appreciate some guidance so I can move forward with this.

Sun, Apr 21, 1:33 PM · Restricted Project
nridge added a comment to D59756: [clangd] Support dependent bases in type hierarchy.

Ping - could this be committed please?

Sun, Apr 21, 1:32 PM · Restricted Project, Restricted Project
nridge created D60953: [clangd] Respect clang-tidy suppression comments.
Sun, Apr 21, 1:31 PM · Restricted Project, Restricted Project

Apr 11 2019

nridge added inline comments to D59756: [clangd] Support dependent bases in type hierarchy.
Apr 11 2019, 8:50 PM · Restricted Project, Restricted Project

Apr 6 2019

nridge added inline comments to D59756: [clangd] Support dependent bases in type hierarchy.
Apr 6 2019, 4:12 PM · Restricted Project, Restricted Project

Apr 5 2019

nridge updated the diff for D59756: [clangd] Support dependent bases in type hierarchy.

Address review comments

Apr 5 2019, 9:20 PM · Restricted Project, Restricted Project
nridge added a comment to D59407: [clangd] Add RelationSlab.

One thing that strikes me here is that this case is very similar to our existing Ref data - it's basically a subset, but with a symbolid payload instead of location. We could consider just adding the SymbolID to Refs - it'd blow up the size of that by 50%, but we may not do much better with some other representation, and it would avoid adding any new complexity.

Apr 5 2019, 8:26 PM · Restricted Project

Apr 1 2019

nridge requested review of D59407: [clangd] Add RelationSlab.

I guess I should clear the "Accepted" status until we settle the question of the data model.

Apr 1 2019, 9:44 PM · Restricted Project

Mar 31 2019

nridge added a comment to D60040: [clangd] Use capacity() instead of size() in RefSlab::bytes().

I do not.

Mar 31 2019, 11:21 AM · Restricted Project, Restricted Project

Mar 30 2019

nridge added a comment to D60040: [clangd] Use capacity() instead of size() in RefSlab::bytes().

This was suggested in this comment.

Mar 30 2019, 8:14 PM · Restricted Project, Restricted Project
nridge created D60040: [clangd] Use capacity() instead of size() in RefSlab::bytes().
Mar 30 2019, 8:13 PM · Restricted Project, Restricted Project
nridge abandoned D57560: [clangd] Link libclangAST into clangd tool.

Abandoning as this has been fixed.

Mar 30 2019, 7:04 PM · Restricted Project, Restricted Project

Mar 28 2019

nridge added a comment to D59407: [clangd] Add RelationSlab.

@sammccall, thank you for having a look at this.

Mar 28 2019, 6:14 PM · Restricted Project

Mar 26 2019

nridge added a comment to D59407: [clangd] Add RelationSlab.

Ok, cool. In that case, I think this patch is ready to be committed, and would appreciate it if someone could commit it. Thanks!

Mar 26 2019, 5:40 PM · Restricted Project
nridge added a comment to D59407: [clangd] Add RelationSlab.

As this is the first of a series of patches adding support for relations, and then building type hierarchy subtypes on top (as discussed here), how should we go about landing this -- should we land each patch in the series as soon as it's ready, or should we wait to land them all together?

Mar 26 2019, 4:31 PM · Restricted Project

Mar 24 2019

nridge added a comment to D59756: [clangd] Support dependent bases in type hierarchy.

This patch aims to implement the more proper solution suggested here.

Mar 24 2019, 8:01 PM · Restricted Project, Restricted Project
nridge created D59756: [clangd] Support dependent bases in type hierarchy.
Mar 24 2019, 7:58 PM · Restricted Project, Restricted Project

Mar 22 2019

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

Scrapped 'struct Relation' and addressed other comments

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

Mar 21 2019

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

Mar 17 2019

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

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

Mar 17 2019, 12:21 PM · Restricted Project
nridge added inline comments to D59407: [clangd] Add RelationSlab.
Mar 17 2019, 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.

Mar 17 2019, 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.

Mar 17 2019, 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!

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

Mar 14 2019

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
Mar 14 2019, 10:31 PM · Restricted Project
nridge added inline comments to D59407: [clangd] Add RelationSlab.
Mar 14 2019, 10:31 PM · Restricted Project
nridge created D59407: [clangd] Add RelationSlab.
Mar 14 2019, 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

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

Mar 12 2019

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

Address remaining review comments

Mar 12 2019, 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.

Mar 12 2019, 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.

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

Mar 11 2019

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.

Mar 11 2019, 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?

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

Mar 10 2019

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.

Mar 10 2019, 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

Mar 10 2019, 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.

Mar 10 2019, 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

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

Mar 7 2019

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

Thanks for the detailed analysis!

Mar 7 2019, 6:02 PM · Restricted Project

Mar 5 2019

nridge added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Mar 5 2019, 7:56 PM · Restricted Project, Restricted Project
nridge added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Mar 5 2019, 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.

Mar 5 2019, 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
Mar 5 2019, 11:13 AM · Restricted Project

Mar 4 2019

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?

Mar 4 2019, 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

Mar 4 2019, 9:52 PM · Restricted Project

Mar 3 2019

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

Address a couple of outstanding TODOs

Mar 3 2019, 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.

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

Rebased onto updated D56370

Mar 3 2019, 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

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

Mar 2 2019

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).
Mar 2 2019, 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.
Mar 2 2019, 5:23 PM · Restricted Project, Restricted Project
nridge created D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes.
Mar 2 2019, 5:23 PM · Restricted Project
nridge updated the diff for D56370: [clangd] Add support for type hierarchy (super types only for now).

Rebased.

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

Feb 26 2019

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

Feb 26 2019, 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