Page MenuHomePhabricator

nridge (Nathan Ridge)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 4 2018, 8:41 PM (147 w, 3 d)

Recent Activity

Sun, Nov 29

nridge committed rGf15b7869e5af: [clang-tidy] [clangd] Avoid multi-line diagnostic range for else-after-return… (authored by nridge).
[clang-tidy] [clangd] Avoid multi-line diagnostic range for else-after-return…
Sun, Nov 29, 3:33 PM
nridge closed D92272: [clang-tidy] [clangd] Avoid multi-line diagnostic range for else-after-return diagnostic.
Sun, Nov 29, 3:33 PM · Restricted Project
nridge added a comment to D91025: [clangd] Fix locateMacroAt() for macro definition outside preamble.

Review ping :)

Sun, Nov 29, 3:11 PM · Restricted Project
nridge requested review of D92290: [clangd] Use clangd's Context mechanism to make the ASTContext of the AST being operated on available everywhere.
Sun, Nov 29, 3:06 PM · Restricted Project
nridge updated the diff for D92272: [clang-tidy] [clangd] Avoid multi-line diagnostic range for else-after-return diagnostic.

Rebase and address review comment

Sun, Nov 29, 2:57 PM · Restricted Project

Sat, Nov 28

nridge added a comment to D92272: [clang-tidy] [clangd] Avoid multi-line diagnostic range for else-after-return diagnostic.

I see that you have a more sophisticated fix at D91149. Will leave it up to you if we should just wait for that to land, or land this in the interim.

Sat, Nov 28, 5:04 PM · Restricted Project
nridge requested review of D92272: [clang-tidy] [clangd] Avoid multi-line diagnostic range for else-after-return diagnostic.
Sat, Nov 28, 4:57 PM · Restricted Project

Thu, Nov 26

nridge added a comment to D92155: Load plugins when creating a CompilerInvocation..

What I'm asking specifically is: this feature has a cost, how important is supporting it? Are there codebases where these attributes are widely used, and enforcement at development time is particularly valuable?

Thu, Nov 26, 10:04 PM · Restricted Project
nridge committed rGd1fd91ddaf9d: [clangd] Do not treat line as inactive if skipped range ends at character… (authored by nridge).
[clangd] Do not treat line as inactive if skipped range ends at character…
Thu, Nov 26, 12:43 AM
nridge closed D92148: [clangd] Do not treat line as inactive if skipped range ends at character position 0.
Thu, Nov 26, 12:43 AM · Restricted Project

Wed, Nov 25

nridge requested review of D92148: [clangd] Do not treat line as inactive if skipped range ends at character position 0.
Wed, Nov 25, 6:31 PM · Restricted Project
nridge committed rGc6cb47b640ff: [clangd] Collect main file refs by default (authored by nridge).
[clangd] Collect main file refs by default
Wed, Nov 25, 5:34 PM
nridge closed D92000: [clangd] Collect main file refs by default.
Wed, Nov 25, 5:34 PM · Restricted Project
nridge updated the diff for D92000: [clangd] Collect main file refs by default.

Address review comment

Wed, Nov 25, 5:32 PM · Restricted Project
nridge added a comment to D92000: [clangd] Collect main file refs by default.

One thing that just occurred to me: does this need an index version bump?

Wed, Nov 25, 11:53 AM · Restricted Project
nridge committed rG3d2c681f2835: [clangd] Avoid type hierarchy crash on incomplete type (authored by nridge).
[clangd] Avoid type hierarchy crash on incomplete type
Wed, Nov 25, 12:45 AM
nridge closed D92077: [clangd] Avoid type hierarchy crash on incomplete type.
Wed, Nov 25, 12:45 AM · Restricted Project
nridge requested review of D92077: [clangd] Avoid type hierarchy crash on incomplete type.
Wed, Nov 25, 12:12 AM · Restricted Project

Tue, Nov 24

nridge committed rG5b6f47595bab: [clangd] Sort results of incomingCalls request by container name (authored by nridge).
[clangd] Sort results of incomingCalls request by container name
Tue, Nov 24, 12:29 AM
nridge closed D92009: [clangd] Sort results of incomingCalls request by container name.
Tue, Nov 24, 12:29 AM · Restricted Project
nridge updated the diff for D92009: [clangd] Sort results of incomingCalls request by container name.

Address review comments

Tue, Nov 24, 12:28 AM · Restricted Project
nridge added a comment to D92000: [clangd] Collect main file refs by default.

Note, the call hierarchy feature is pretty significantly impaired without this. See the example of Selection::createEach() discussed in this comment.

Tue, Nov 24, 12:23 AM · Restricted Project
nridge added inline comments to D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).
Tue, Nov 24, 12:22 AM · Restricted Project
nridge added inline comments to D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).
Tue, Nov 24, 12:21 AM · Restricted Project
nridge requested review of D92009: [clangd] Sort results of incomingCalls request by container name.
Tue, Nov 24, 12:18 AM · Restricted Project

Mon, Nov 23

nridge added a comment to D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).

It looks like CallHierarchy.IncomingOneFile in failing on a few of the buildbots (e.g. clang-ppc64be-linux and clang-s390x-linux), while passing on most of them. I cannot reproduce the failure locally.

Mon, Nov 23, 10:32 PM · Restricted Project
nridge requested review of D92000: [clangd] Collect main file refs by default.
Mon, Nov 23, 6:00 PM · Restricted Project
nridge committed rGdced150375d0: [clangd] Use WorkScheduler.run() in ClangdServer::resolveTypeHierarchy() (authored by nridge).
[clangd] Use WorkScheduler.run() in ClangdServer::resolveTypeHierarchy()
Mon, Nov 23, 5:45 PM
nridge committed rG0a4f99c494d0: [clangd] Call hierarchy (ClangdLSPServer layer) (authored by nridge).
[clangd] Call hierarchy (ClangdLSPServer layer)
Mon, Nov 23, 5:45 PM
nridge committed rG4cb976e014db: [clangd] Call hierarchy (ClangdServer layer) (authored by nridge).
[clangd] Call hierarchy (ClangdServer layer)
Mon, Nov 23, 5:45 PM
nridge committed rG3e6e6a2db674: [clangd] Call hierarchy (XRefs layer, incoming calls) (authored by nridge).
[clangd] Call hierarchy (XRefs layer, incoming calls)
Mon, Nov 23, 5:45 PM
nridge closed D91941: [clangd] Use WorkScheduler.run() in ClangdServer::resolveTypeHierarchy().
Mon, Nov 23, 5:44 PM · Restricted Project
nridge closed D91124: [clangd] Call hierarchy (ClangdLSPServer layer).
Mon, Nov 23, 5:44 PM · Restricted Project
nridge closed D91123: [clangd] Call hierarchy (ClangdServer layer).
Mon, Nov 23, 5:44 PM · Restricted Project
nridge closed D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).
Mon, Nov 23, 5:44 PM · Restricted Project
nridge added a comment to D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).

Thank you for the reviews!

Mon, Nov 23, 5:42 PM · Restricted Project
nridge updated the diff for D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).

Address final review comment

Mon, Nov 23, 5:42 PM · Restricted Project

Sun, Nov 22

nridge added a comment to D91941: [clangd] Use WorkScheduler.run() in ClangdServer::resolveTypeHierarchy().

As discussed in https://reviews.llvm.org/D91123#inline-856770

Sun, Nov 22, 6:48 PM · Restricted Project
nridge requested review of D91941: [clangd] Use WorkScheduler.run() in ClangdServer::resolveTypeHierarchy().
Sun, Nov 22, 6:47 PM · Restricted Project
nridge updated the diff for D91124: [clangd] Call hierarchy (ClangdLSPServer layer).

Update as per API changes in xrefs patch

Sun, Nov 22, 6:40 PM · Restricted Project
nridge updated the diff for D91123: [clangd] Call hierarchy (ClangdServer layer).

Update as per API changes in xrefs patch

Sun, Nov 22, 6:37 PM · Restricted Project
nridge added inline comments to D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).
Sun, Nov 22, 6:28 PM · Restricted Project
nridge updated the diff for D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).

Address review comments

Sun, Nov 22, 6:26 PM · Restricted Project
nridge abandoned D91940: [llvm-profgen] Fix shared-lib builds.

You're right, the change to add AllTargetsInfos already fixed this. I will abandon this patch.

Sun, Nov 22, 5:34 PM · Restricted Project
nridge added a comment to D91940: [llvm-profgen] Fix shared-lib builds.

Without this change, a shared-libraries build of LLVM fails for me with the error:

Sun, Nov 22, 4:57 PM · Restricted Project
nridge added a reviewer for D91940: [llvm-profgen] Fix shared-lib builds: wlei.
Sun, Nov 22, 4:56 PM · Restricted Project
nridge requested review of D91940: [llvm-profgen] Fix shared-lib builds.
Sun, Nov 22, 4:55 PM · Restricted Project
nridge added a comment to D91859: [clangd] Fix shared-lib builds.

Ah, right. I think clangd-remote-index needs protobuf dependency. @nridge, I believe this should fix the problem you're seeing.

Sun, Nov 22, 1:10 PM · Restricted Project, Restricted Project
nridge added a comment to D91859: [clangd] Fix shared-lib builds.

I applied the patch locally and it fixes most of the linker errors but I'm still seeing one:

[...]

I am not able to reproduce the failure. Maybe we should just make the dependencies introduced in generate_protos for grpc++ and protobuf PUBLIC, to ensure they propagate into users (i believe that's the default for my configuration for whatever reason). Can you share your cmake configuration ?

Sun, Nov 22, 12:51 PM · Restricted Project, Restricted Project

Sat, Nov 21

nridge added a comment to D91859: [clangd] Fix shared-lib builds.

I applied the patch locally and it fixes most of the linker errors but I'm still seeing one:

Sat, Nov 21, 11:15 AM · Restricted Project, Restricted Project

Wed, Nov 18

nridge added inline comments to D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).
Wed, Nov 18, 12:49 AM · Restricted Project
nridge committed rG9d77584fe040: [clangd] Call hierarchy (Protocol layer) (authored by nridge).
[clangd] Call hierarchy (Protocol layer)
Wed, Nov 18, 12:42 AM
nridge closed D89296: [clangd] Call hierarchy (Protocol layer).
Wed, Nov 18, 12:42 AM · Restricted Project, Restricted Project

Sun, Nov 15

nridge updated the diff for D91124: [clangd] Call hierarchy (ClangdLSPServer layer).

Address review comments

Sun, Nov 15, 8:09 PM · Restricted Project
nridge added a comment to D91123: [clangd] Call hierarchy (ClangdServer layer).

can you also add some tests to ClangdTests.cpp ?

Sun, Nov 15, 8:08 PM · Restricted Project
nridge updated the diff for D91123: [clangd] Call hierarchy (ClangdServer layer).

Address review comments

Sun, Nov 15, 8:08 PM · Restricted Project
nridge updated the diff for D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).

Update variable names and remove a commented declaration

Sun, Nov 15, 8:07 PM · Restricted Project
nridge updated the diff for D89296: [clangd] Call hierarchy (Protocol layer).

Forgot to rename a few fields

Sun, Nov 15, 7:52 PM · Restricted Project, Restricted Project
nridge added inline comments to D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).
Sun, Nov 15, 6:18 PM · Restricted Project
nridge updated the diff for D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).

Address review comments

Sun, Nov 15, 6:17 PM · Restricted Project
nridge updated the diff for D89296: [clangd] Call hierarchy (Protocol layer).

Address review comments

Sun, Nov 15, 4:36 PM · Restricted Project, Restricted Project
nridge added a reviewer for D91025: [clangd] Fix locateMacroAt() for macro definition outside preamble: sammccall.

Sam, you're my go-to reviewer for "tricky macro stuff", but feel free to hand off if you prefer :)

Sun, Nov 15, 3:41 PM · Restricted Project
nridge added a comment to D91131: [clangd] Bump index version number..

My apologies for not thinking of this.

Sun, Nov 15, 3:33 PM · Restricted Project

Thu, Nov 12

nridge accepted D91351: [tooling] Implement determinsitic ordering of CompilationDatabasePlugins.

Not sure if I'm authorized to approve changes to libTooling, but this LGTM!

Thu, Nov 12, 6:45 PM · Restricted Project
nridge added a comment to D91351: [tooling] Implement determinsitic ordering of CompilationDatabasePlugins.

Thanks for working on this!

Thu, Nov 12, 5:04 PM · Restricted Project

Tue, Nov 10

nridge added inline comments to D91186: [clangd] Add documentation for building and testing clangd.
Tue, Nov 10, 4:28 PM · Restricted Project
nridge added a comment to D91124: [clangd] Call hierarchy (ClangdLSPServer layer).

Thanks for the reviews!

Tue, Nov 10, 4:23 PM · Restricted Project

Mon, Nov 9

nridge requested review of D91124: [clangd] Call hierarchy (ClangdLSPServer layer).
Mon, Nov 9, 9:58 PM · Restricted Project
nridge requested review of D91123: [clangd] Call hierarchy (ClangdServer layer).
Mon, Nov 9, 9:57 PM · Restricted Project
nridge updated the diff for D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).

Update a comment

Mon, Nov 9, 9:56 PM · Restricted Project
nridge requested review of D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).
Mon, Nov 9, 9:53 PM · Restricted Project
nridge retitled D89296: [clangd] Call hierarchy (Protocol layer) from [clangd] Implement call hierarchy (incoming calls) to [clangd] Call hierarchy (Protocol layer).
Mon, Nov 9, 9:53 PM · Restricted Project, Restricted Project
nridge updated the diff for D89296: [clangd] Call hierarchy (Protocol layer).

Split patch as requested

Mon, Nov 9, 9:52 PM · Restricted Project, Restricted Project

Sun, Nov 8

nridge added a reviewer for D89296: [clangd] Call hierarchy (Protocol layer): kadircet.

Kadir, would you like to take a look at this, as you're already familiar with the index changes? :)

Sun, Nov 8, 5:11 PM · Restricted Project, Restricted Project

Sat, Nov 7

nridge requested review of D91025: [clangd] Fix locateMacroAt() for macro definition outside preamble.
Sat, Nov 7, 11:59 PM · Restricted Project

Wed, Nov 4

nridge committed rG3bec07f91fb1: [clangd] Store the containing symbol for refs (authored by nridge).
[clangd] Store the containing symbol for refs
Wed, Nov 4, 12:22 AM
nridge closed D89670: [clangd] Store the containing symbol for refs.
Wed, Nov 4, 12:21 AM · Restricted Project
nridge updated the diff for D89670: [clangd] Store the containing symbol for refs.

Address final review comments

Wed, Nov 4, 12:20 AM · Restricted Project

Nov 2 2020

nridge added a comment to D89670: [clangd] Store the containing symbol for refs.

(I will wait for a response about the containers for top-level decls before committing.)

Nov 2 2020, 11:31 PM · Restricted Project
nridge updated the diff for D89670: [clangd] Store the containing symbol for refs.

Address review comments

Nov 2 2020, 11:28 PM · Restricted Project
nridge added inline comments to D89670: [clangd] Store the containing symbol for refs.
Nov 2 2020, 11:17 PM · Restricted Project

Oct 31 2020

nridge added inline comments to D89670: [clangd] Store the containing symbol for refs.
Oct 31 2020, 11:32 PM · Restricted Project
nridge updated the diff for D89670: [clangd] Store the containing symbol for refs.

Address review comments

Oct 31 2020, 11:32 PM · Restricted Project
nridge added a comment to D90397: [clangd] Value initialize SymbolIDs.

Would it have been possible to disallow default-constructing SymbolID altogether, and preserve the ability to represent "an always-valid symbol id" (SymbolID) vs. "a maybe-valid symbol id" (Optional<SymbolID>) as distinct types in the type system?

Absolutely, except where it matters`sizeof(SymbolID)==8` and sizeof(Optional<SymbolID>)==16.
I think the trigger here was SymbolID Ref::Container - we can't afford to use Optional there.

Oct 31 2020, 10:13 PM · Restricted Project
nridge added a comment to D90397: [clangd] Value initialize SymbolIDs.

Would it have been possible to disallow default-constructing SymbolID altogether, and preserve the ability to represent "an always-valid symbol id" (SymbolID) vs. "a maybe-valid symbol id" (Optional<SymbolID>) as distinct types in the type system?

Oct 31 2020, 3:58 PM · Restricted Project

Oct 26 2020

nridge committed rG245b61a330ab: [clangd] Increase the TooMany limit for index-based textual navigation to 5 (authored by nridge).
[clangd] Increase the TooMany limit for index-based textual navigation to 5
Oct 26 2020, 10:49 PM
nridge closed D90134: [clangd] Increase the TooMany limit for index-based textual navigation to 5.
Oct 26 2020, 10:49 PM · Restricted Project
nridge added a comment to D90134: [clangd] Increase the TooMany limit for index-based textual navigation to 5.

My experience using this feature in real codebases has been that we take this early-exit too often; the limit of 3 is fairly easy to hit due to things like layers of wrappers that repeat a function name. Bumping the limit to 5 has increased the usefulness for me.

What about counting number of different files, rather than number of symbols ?

Oct 26 2020, 10:49 PM · Restricted Project
nridge committed rG2756e2ee0bce: [libTooling] Recognize sccache as a compiler wrapper in compilation database… (authored by nridge).
[libTooling] Recognize sccache as a compiler wrapper in compilation database…
Oct 26 2020, 10:47 PM
nridge closed D88790: [libTooling] Recognize sccache as a compiler wrapper in compilation database commands.
Oct 26 2020, 10:47 PM · Restricted Project
nridge abandoned D78784: [clangd] Add some logging to explain why textual fallback navigation failed.

Abandoning as per discussion. Running this locally has led me to propose D90134.

Oct 26 2020, 12:26 AM · Restricted Project
nridge added a comment to D90134: [clangd] Increase the TooMany limit for index-based textual navigation to 5.

My experience using this feature in real codebases has been that we take this early-exit too often; the limit of 3 is fairly easy to hit due to things like layers of wrappers that repeat a function name. Bumping the limit to 5 has increased the usefulness for me.

Oct 26 2020, 12:25 AM · Restricted Project
nridge requested review of D90134: [clangd] Increase the TooMany limit for index-based textual navigation to 5.
Oct 26 2020, 12:22 AM · Restricted Project
nridge added a comment to D88790: [libTooling] Recognize sccache as a compiler wrapper in compilation database commands.

Review ping :)

Oct 26 2020, 12:19 AM · Restricted Project

Oct 25 2020

nridge added a comment to D89296: [clangd] Call hierarchy (Protocol layer).

Note, testing this with vscode requires this vscode-clangd patch applied (to use a newer version of the client libraries that support the data field).

Oct 25 2020, 11:39 AM · Restricted Project, Restricted Project
nridge published D89296: [clangd] Call hierarchy (Protocol layer) for review.

Keeping in the draft state for now, as I still need to address this comment from the issue discussion:

The request still has a progressToken attached to it, I presume(it is unclear from the spec) it is preserved between prepare and subsequent requests. So we can keep a mapping in ClangdServer and stash symbolIDs.

Oct 25 2020, 11:36 AM · Restricted Project, Restricted Project

Oct 24 2020

nridge added a reviewer for D89670: [clangd] Store the containing symbol for refs: kadircet.
Oct 24 2020, 6:49 PM · Restricted Project
nridge added inline comments to D89670: [clangd] Store the containing symbol for refs.
Oct 24 2020, 6:49 PM · Restricted Project
nridge updated the diff for D89670: [clangd] Store the containing symbol for refs.

Use Decl rather than NamedDecl, and add a test

Oct 24 2020, 6:47 PM · Restricted Project
nridge committed rGaaa8b44d1991: [clangd] Add a TestWorkspace utility (authored by nridge).
[clangd] Add a TestWorkspace utility
Oct 24 2020, 5:16 PM