Page MenuHomePhabricator

nridge (Nathan Ridge)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Wed, Jan 20

nridge added inline comments to D92290: [clangd] Factor out the heuristic resolver code into its own class.
Wed, Jan 20, 11:52 AM · Restricted Project
nridge added a comment to D95031: [clangd] Log warning when using legacy (theia) semantic highlighting..

Thanks for the heads up! It would be nice to get D77702 landed before the removal of TheiaSemanticHighlighting. I'll try to find some time to rebase it.

Wed, Jan 20, 8:37 AM · Restricted Project

Mon, Jan 18

nridge added a comment to D92290: [clangd] Factor out the heuristic resolver code into its own class.

Thanks for the comments! I'll have a more detailed look later, but just wanted to ask one clarifying question for now:

Mon, Jan 18, 10:46 AM · Restricted Project
nridge updated the summary of D92290: [clangd] Factor out the heuristic resolver code into its own class.
Mon, Jan 18, 12:11 AM · Restricted Project
nridge retitled D92290: [clangd] Factor out the heuristic resolver code into its own class from [clangd] Use clangd's Context mechanism to make the ASTContext of the AST being operated on available everywhere to [clangd] Factor out the heuristic resolver code into its own class.
Mon, Jan 18, 12:11 AM · Restricted Project
nridge updated the diff for D92290: [clangd] Factor out the heuristic resolver code into its own class.

Change approach to factor out a HeuristicResolver class

Mon, Jan 18, 12:11 AM · Restricted Project

Sun, Jan 17

nridge added a comment to D94785: [clangd] Index local classes, virtual and overriding methods..

Although now that I enabled this in all SymbolCollectorTests, this causes a regression in RefContainers.

void f2() {
      (void) $ref1a[[f1]](1);
      auto fptr = &$ref1b[[f1]];
    }

&f1 is contained in fptr instead of f2. No immediate ideas why would this happen.

Sun, Jan 17, 3:10 PM · Restricted Project

Tue, Jan 12

nridge committed rG4718ec01669b: [clangd] Avoid recursion in TargetFinder::add() (authored by nridge).
[clangd] Avoid recursion in TargetFinder::add()
Tue, Jan 12, 10:58 AM
nridge closed D94382: [clangd] Avoid recursion in TargetFinder::add().
Tue, Jan 12, 10:58 AM · Restricted Project
nridge added a comment to D93829: [clangd] Support outgoing calls in call hierarchy.

I think that, as with incoming calls, the incremental value is in seeing the tree at a glance. So, you might query the outgoing calls for a function, expand the tree, and look over the transitive callees to see if the function ends up performing a certain type of operation (say, a filesystem access).

Tue, Jan 12, 10:14 AM · Restricted Project

Mon, Jan 11

nridge updated the diff for D94382: [clangd] Avoid recursion in TargetFinder::add().

Address review comments

Mon, Jan 11, 11:37 PM · Restricted Project
nridge planned changes to D92290: [clangd] Factor out the heuristic resolver code into its own class.

That all sounds reasonable -- thanks for the suggestions!

Mon, Jan 11, 12:00 AM · Restricted Project

Sun, Jan 10

nridge added a comment to D93829: [clangd] Support outgoing calls in call hierarchy.

Thanks for working on this!

Sun, Jan 10, 11:57 PM · Restricted Project
nridge added inline comments to D94382: [clangd] Avoid recursion in TargetFinder::add().
Sun, Jan 10, 11:06 PM · Restricted Project
nridge updated the diff for D94382: [clangd] Avoid recursion in TargetFinder::add().

Use map lookup instead of iteration

Sun, Jan 10, 6:29 PM · Restricted Project
nridge added inline comments to D94382: [clangd] Avoid recursion in TargetFinder::add().
Sun, Jan 10, 6:29 PM · Restricted Project
nridge added inline comments to D94382: [clangd] Avoid recursion in TargetFinder::add().
Sun, Jan 10, 5:53 PM · Restricted Project
nridge requested review of D94382: [clangd] Avoid recursion in TargetFinder::add().
Sun, Jan 10, 5:49 PM · Restricted Project

Dec 18 2020

nridge added a comment to D93522: [WIP] [clangd] Extend heuristic resolution to support nested templates.

(This is not ready for review, I just posted it in case it helps inform discussion in D92290.)

Dec 18 2020, 12:35 AM · Restricted Project
nridge added a comment to D92290: [clangd] Factor out the heuristic resolver code into its own class.

my manager was appalled when we added it...

Dec 18 2020, 12:33 AM · Restricted Project
nridge requested review of D93522: [WIP] [clangd] Extend heuristic resolution to support nested templates.
Dec 18 2020, 12:26 AM · Restricted Project

Dec 15 2020

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

I thought getConfiguration('clangd') with no scope specified was supposed to be global (i.e. not a workspace-specific setting). There's a scope you can pass in, and we're not providing one.

Nevertheless testing it locally these flags do seem to be used. We should fix this, I think workspace.getConfiguration('clangd').inspect('arguments') and then applying the components manually makes it possible. This is a horrible breaking change, though :-(

Dec 15 2020, 12:45 AM · Restricted Project

Dec 13 2020

nridge updated the diff for D92290: [clangd] Factor out the heuristic resolver code into its own class.

Add a missed call site

Dec 13 2020, 4:28 PM · Restricted Project
nridge added reviewers for D92290: [clangd] Factor out the heuristic resolver code into its own class: sammccall, hokein, kadircet.

Shopping the patch around to some possible reviewers.

Dec 13 2020, 3:36 PM · Restricted Project
nridge committed rGfef242c32e83: [clangd] Fix locateMacroAt() for macro definition outside preamble (authored by nridge).
[clangd] Fix locateMacroAt() for macro definition outside preamble
Dec 13 2020, 3:34 PM
nridge closed D91025: [clangd] Fix locateMacroAt() for macro definition outside preamble.
Dec 13 2020, 3:34 PM · Restricted Project
nridge updated the diff for D91025: [clangd] Fix locateMacroAt() for macro definition outside preamble.

Address review comments

Dec 13 2020, 3:32 PM · Restricted Project

Dec 12 2020

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

Or, could you help to point out what's the difference between passing a plugin path through *clangd* startup command line and through clang flags?

Sure. TL;DR is: clangd flags are configured by the user, user can be fully responsible for security/stability.
clang flags are configured by the project. If they're bad, we can e.g. give bad diagnostics, but can't crash or compromise security.

More detail:

In the simplest possible case, clangd is configured as follows:

  1. user downloads clangd binary
  2. user installs an LSP plugin for their editor, and configures the plugin to use /usr/bin/clangd for C++ files. clangd starts when the editor does
  3. the build system for $PROJECT generates $PROJECT/compile_commands.json
  4. when the user opens $PROJECT/src/foo.cpp in the editor, it notifies clangd. clangd searches for $PROJECT/compile_commands.json, finds the clang arguments, and uses them to parse foo.cpp

*clangd* command-line flags would be added explicitly by the user at step 2. We can reasonably ask the user to be aware/responsible for security/stability implications of doing this, including with their particular clangd version. We can also ask them to run clangd --check without the plugin flag to test whether the plugin is causing a stability problem.

*clang* command-line flags are added implicitly in step 3. Or they could simply be checked into the repository - nothing ensures they were generated locally by the build system. The point is in typical usage they are not controlled by the user directly, and from a security perspective are not trusted (as safely opening files from untrusted repos is a reasonable expectation). So if we're loading plugins based on instructions in clang command-line flags, clangd bears most of the responsibility for making sure that's safe and correct (and I don't see a way to do that).

Dec 12 2020, 12:16 PM · Restricted Project

Nov 29 2020

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…
Nov 29 2020, 3:33 PM
nridge closed D92272: [clang-tidy] [clangd] Avoid multi-line diagnostic range for else-after-return diagnostic.
Nov 29 2020, 3:33 PM · Restricted Project
nridge added a comment to D91025: [clangd] Fix locateMacroAt() for macro definition outside preamble.

Review ping :)

Nov 29 2020, 3:11 PM · Restricted Project
nridge requested review of D92290: [clangd] Factor out the heuristic resolver code into its own class.
Nov 29 2020, 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

Nov 29 2020, 2:57 PM · Restricted Project

Nov 28 2020

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.

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

Nov 26 2020

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?

Nov 26 2020, 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…
Nov 26 2020, 12:43 AM
nridge closed D92148: [clangd] Do not treat line as inactive if skipped range ends at character position 0.
Nov 26 2020, 12:43 AM · Restricted Project

Nov 25 2020

nridge requested review of D92148: [clangd] Do not treat line as inactive if skipped range ends at character position 0.
Nov 25 2020, 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
Nov 25 2020, 5:34 PM
nridge closed D92000: [clangd] Collect main file refs by default.
Nov 25 2020, 5:34 PM · Restricted Project
nridge updated the diff for D92000: [clangd] Collect main file refs by default.

Address review comment

Nov 25 2020, 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?

Nov 25 2020, 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
Nov 25 2020, 12:45 AM
nridge closed D92077: [clangd] Avoid type hierarchy crash on incomplete type.
Nov 25 2020, 12:45 AM · Restricted Project
nridge requested review of D92077: [clangd] Avoid type hierarchy crash on incomplete type.
Nov 25 2020, 12:12 AM · Restricted Project

Nov 24 2020

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

Address review comments

Nov 24 2020, 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.

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

Nov 23 2020

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.

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

Thank you for the reviews!

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

Address final review comment

Nov 23 2020, 5:42 PM · Restricted Project

Nov 22 2020

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

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

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

Update as per API changes in xrefs patch

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

Update as per API changes in xrefs patch

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

Address review comments

Nov 22 2020, 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.

Nov 22 2020, 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:

Nov 22 2020, 4:57 PM · Restricted Project
nridge added a reviewer for D91940: [llvm-profgen] Fix shared-lib builds: wlei.
Nov 22 2020, 4:56 PM · Restricted Project
nridge requested review of D91940: [llvm-profgen] Fix shared-lib builds.
Nov 22 2020, 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.

Nov 22 2020, 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 ?

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

Nov 21 2020

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:

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

Nov 18 2020

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

Nov 15 2020

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

Address review comments

Nov 15 2020, 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 ?

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

Address review comments

Nov 15 2020, 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

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

Forgot to rename a few fields

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

Address review comments

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

Address review comments

Nov 15 2020, 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 :)

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

My apologies for not thinking of this.

Nov 15 2020, 3:33 PM · Restricted Project

Nov 12 2020

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

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

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

Thanks for working on this!

Nov 12 2020, 5:04 PM · Restricted Project

Nov 10 2020

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

Thanks for the reviews!

Nov 10 2020, 4:23 PM · Restricted Project

Nov 9 2020

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

Update a comment

Nov 9 2020, 9:56 PM · Restricted Project
nridge requested review of D91122: [clangd] Call hierarchy (XRefs layer, incoming calls).
Nov 9 2020, 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).
Nov 9 2020, 9:53 PM · Restricted Project, Restricted Project