Page MenuHomePhabricator

nridge (Nathan Ridge)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Fri, Nov 25

nridge added a comment to D136594: [clangd] Add support for semantic token type "operator".

(I envisioned the implementation of augmentsSyntaxTokens=false to be "loop over the lexer tokens and turn a subset of them into additional tokens to return over LSP". The fact that that would have meant producing an "operator" token even for tok::star which is actually a declarator, is what clued me in to the fact that "operators" are a semantic category.)

Fri, Nov 25, 1:30 AM · Restricted Project, Restricted Project
nridge added a comment to D136594: [clangd] Add support for semantic token type "operator".

An (IMO) useful distinction that can't be found by the lexer is the use of * as a declarator (int *x) vs an operator (return *x).

Fri, Nov 25, 1:19 AM · Restricted Project, Restricted Project
nridge added a comment to D138546: Clangd: Preserve target flags in system includes extractor.

Sam, my read, too, is that the memoizing design isn't safe--also that the key bug is preexisting.
Nathan and I were thinking, though, that we'd should post this incremental fix for review rather than getting bogged down in trying to fix multiple things atomically.

Sure. At first glance the design looks like it's been changed in a way that's broken, but maybe there's some deeper reason that it's safe. That reason may or may not also apply to -target (e.g. if -target could plausibly differ across a project but other flags couldn't). I wanted to understand whether/why it's broken today before concluding it's safe to break it further. Probably @kadircet is the best person to make a call here.

Fri, Nov 25, 12:55 AM · Restricted Project, Restricted Project

Wed, Nov 23

nridge added a comment to D137770: [docs] Introduce clangd part in StandardCPlusPlusModules.

I realised this is an active issue that's not being worked on. So, I want to put myself forward to help implement this new module system for clangd once you have settled on a design/approach.

Wed, Nov 23, 12:49 AM · Restricted Project, Restricted Project

Mon, Nov 21

nridge added inline comments to D136594: [clangd] Add support for semantic token type "operator".
Mon, Nov 21, 2:08 PM · Restricted Project, Restricted Project
nridge added a comment to D136594: [clangd] Add support for semantic token type "operator".

I think since LSP specifies an operator SemanticTokenType, we should use it unless there's a *really* strong reason not to.

Mon, Nov 21, 2:06 PM · Restricted Project, Restricted Project
nridge added a comment to D138425: [clangd] Parameter hints for template specialization.

FWIW, in codebases/components where I do code reviews, I tend to insist on template parameters having meaningful (and longer-than-one-character) names, and would find this quite useful.

Mon, Nov 21, 1:58 PM · Restricted Project, Restricted Project

Sun, Nov 20

nridge added a comment to D136594: [clangd] Add support for semantic token type "operator".

A couple of high-level thoughts on this:

Sun, Nov 20, 7:10 PM · Restricted Project, Restricted Project

Sun, Nov 13

nridge accepted D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer.

Thanks!

Sun, Nov 13, 11:43 PM · Restricted Project, Restricted Project, Restricted Project
nridge added a comment to D137697: Move the isSelfContained function from clangd to libtooling..

nit: isSelfContained/isSelfContainedHeader in commit message

Sun, Nov 13, 12:08 AM · Restricted Project, Restricted Project, Restricted Project

Sat, Nov 12

nridge added a comment to D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer.

now we kinda have the same test case duplicated in sema and clangd tests - I guess for clangd we now actually only have to test that the SnippetSuffix is cleared when FunctionCanBeCall is true, but I don't see an easy way to somehow inject fake Sema results into CodeComplete.cpp

Sat, Nov 12, 6:16 PM · Restricted Project, Restricted Project, Restricted Project

Fri, Nov 11

nridge added a comment to D137770: [docs] Introduce clangd part in StandardCPlusPlusModules.

And we need the compiler (clang) to parse the module file from different versions.

Fri, Nov 11, 12:56 AM · Restricted Project, Restricted Project

Tue, Nov 8

nridge abandoned D137674: [clangd] Make system-include-extractor.test more resilient in the face of paths with special characters.

Great minds :)

Tue, Nov 8, 2:55 PM · Restricted Project, Restricted Project
nridge added a comment to D133757: [clangd] Perform system include extraction inside CommandMangler.

This change is causing system-include-extractor.test to fail on Chrome: https://crbug.com/1382508

The test fails on this assertion if the path to the LLVM repository contains the plus sign (+). The actual output replaces the plus sign with the URL escape code %2B, but the still expects the plus sign in the output.

Tue, Nov 8, 2:45 PM · Restricted Project, Restricted Project
nridge requested review of D137674: [clangd] Make system-include-extractor.test more resilient in the face of paths with special characters.
Tue, Nov 8, 2:45 PM · Restricted Project, Restricted Project

Mon, Nov 7

nridge committed rG428ac8f3a0f9: [clangd] Rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp (authored by nridge).
[clangd] Rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp
Mon, Nov 7, 3:03 PM · Restricted Project, Restricted Project
nridge committed rG68e230aa29f7: [clangd] Perform system include extraction inside CommandMangler (authored by nridge).
[clangd] Perform system include extraction inside CommandMangler
Mon, Nov 7, 3:03 PM · Restricted Project, Restricted Project
nridge committed rGafa22c563f12: [clangd] Pass the entire tooling::CompileCommand to CommandMangler (authored by nridge).
[clangd] Pass the entire tooling::CompileCommand to CommandMangler
Mon, Nov 7, 3:03 PM · Restricted Project, Restricted Project
nridge closed D137401: [clangd] Rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp.
Mon, Nov 7, 3:03 PM · Restricted Project, Restricted Project
nridge closed D133757: [clangd] Perform system include extraction inside CommandMangler.
Mon, Nov 7, 3:03 PM · Restricted Project, Restricted Project
nridge closed D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler.
Mon, Nov 7, 3:03 PM · Restricted Project, Restricted Project
nridge accepted D130015: [clangd] Add "usedAsMutablePointer" highlighting modifier.
Mon, Nov 7, 1:43 AM · Restricted Project, Restricted Project
nridge committed rG41fa7d2093e0: [clangd] Fix a small inconsistency in system-include-extractor.test (authored by nridge).
[clangd] Fix a small inconsistency in system-include-extractor.test
Mon, Nov 7, 12:30 AM · Restricted Project, Restricted Project
nridge closed D137056: [clangd] Fix a small inconsistency in system-include-extractor.test.
Mon, Nov 7, 12:30 AM · Restricted Project, Restricted Project
nridge updated the diff for D137056: [clangd] Fix a small inconsistency in system-include-extractor.test.

address nit

Mon, Nov 7, 12:29 AM · Restricted Project, Restricted Project
nridge added a comment to D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer.

We now probably also need a unit test for Sema that tests the new flag, right?

Mon, Nov 7, 12:27 AM · Restricted Project, Restricted Project, Restricted Project

Sat, Nov 5

nridge added a comment to D130015: [clangd] Add "usedAsMutablePointer" highlighting modifier.

IMO the relevant point is the (non-)const-ness of the corresponding parameter, i.e. can the passed object get mutated or not. The fact that there's an ampersand at the call site (which is not even the case if the variable is itself a pointer) does not tell me that.

Sat, Nov 5, 1:21 AM · Restricted Project, Restricted Project
nridge added a reviewer for D131295: [clangd] Implement textDocument/codeLens: kadircet.

Haven't had a chance to look at this yet. I do see that the earier implementation in D91930 was the subject of some design discussions about performance with @kadircet, adding him as an additional reviewer.

Sat, Nov 5, 12:40 AM · Restricted Project, Restricted Project

Fri, Nov 4

nridge added a comment to D133757: [clangd] Perform system include extraction inside CommandMangler.

I will follow up with a patch to rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp.

Fri, Nov 4, 1:43 AM · Restricted Project, Restricted Project
nridge added a comment to D137401: [clangd] Rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp.

Follow-up to D133757

Fri, Nov 4, 1:41 AM · Restricted Project, Restricted Project
nridge updated the diff for D137401: [clangd] Rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp.

fix comment

Fri, Nov 4, 1:41 AM · Restricted Project, Restricted Project
nridge requested review of D137401: [clangd] Rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp.
Fri, Nov 4, 1:40 AM · Restricted Project, Restricted Project
nridge added a comment to D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer.

Thanks for the patch!

Fri, Nov 4, 1:27 AM · Restricted Project, Restricted Project, Restricted Project

Wed, Nov 2

nridge accepted D137104: [clangd] Add scoped enum constants to all-scopes-completion.

Looks good, thanks

Wed, Nov 2, 10:31 AM · Restricted Project, Restricted Project
nridge added a comment to D137104: [clangd] Add scoped enum constants to all-scopes-completion.

The test added in the previous patch, CompletionTest.Enums, needs to be updated to reflect this change (Scoped::Clangd3 now appears as a completion).

Wed, Nov 2, 1:37 AM · Restricted Project, Restricted Project
nridge accepted D136925: [clangd] Index unscoped enums inside classes for code completion.

I don't think we need this much detail in the commit message; I think after the first line it would be sufficient to say:

Wed, Nov 2, 1:35 AM · Restricted Project, Restricted Project

Tue, Nov 1

nridge added inline comments to D136925: [clangd] Index unscoped enums inside classes for code completion.
Tue, Nov 1, 2:51 AM · Restricted Project, Restricted Project
nridge added inline comments to D136925: [clangd] Index unscoped enums inside classes for code completion.
Tue, Nov 1, 2:21 AM · Restricted Project, Restricted Project
nridge added inline comments to D133757: [clangd] Perform system include extraction inside CommandMangler.
Tue, Nov 1, 1:40 AM · Restricted Project, Restricted Project
nridge updated the diff for D133757: [clangd] Perform system include extraction inside CommandMangler.

Improve handling of config file diagnostics in lit test

Tue, Nov 1, 1:38 AM · Restricted Project, Restricted Project
nridge added a comment to D121593: [clangd][WIP] Provide clang-include-cleaner.

was there any progress on this so far? This would be really useful to be a clang-tidy check.

Tue, Nov 1, 1:09 AM · Restricted Project, Restricted Project

Oct 31 2022

nridge added inline comments to D133757: [clangd] Perform system include extraction inside CommandMangler.
Oct 31 2022, 2:11 AM · Restricted Project, Restricted Project
nridge added inline comments to D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler.
Oct 31 2022, 2:06 AM · Restricted Project, Restricted Project
nridge added a comment to D133757: [clangd] Perform system include extraction inside CommandMangler.

I believe this addresses the remaining review comments. I will follow up with a patch to rename QueryDriverDatabase.cpp to SystemIncludeExtractor.cpp.

Oct 31 2022, 2:03 AM · Restricted Project, Restricted Project
nridge retitled D133757: [clangd] Perform system include extraction inside CommandMangler from [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster to [clangd] Perform system include extraction inside CommandMangler.
Oct 31 2022, 1:58 AM · Restricted Project, Restricted Project
nridge updated the diff for D133757: [clangd] Perform system include extraction inside CommandMangler.

reformulate test as lit test
address other review comments

Oct 31 2022, 1:57 AM · Restricted Project, Restricted Project
nridge updated the diff for D137056: [clangd] Fix a small inconsistency in system-include-extractor.test.

fix typo

Oct 31 2022, 12:25 AM · Restricted Project, Restricted Project
nridge updated the diff for D137056: [clangd] Fix a small inconsistency in system-include-extractor.test.

format comment better

Oct 31 2022, 12:25 AM · Restricted Project, Restricted Project
nridge requested review of D137056: [clangd] Fix a small inconsistency in system-include-extractor.test.
Oct 31 2022, 12:22 AM · Restricted Project, Restricted Project

Oct 30 2022

nridge added inline comments to D136925: [clangd] Index unscoped enums inside classes for code completion.
Oct 30 2022, 9:15 PM · Restricted Project, Restricted Project
nridge requested review of D134827: [clangd] Avoid recursion on UnresolvedUsingValueDecl during semantic highlighting.

(I'm going to re-request review since I have an outstanding question about the fix approach.)

Oct 30 2022, 12:56 AM · Restricted Project, Restricted Project
nridge added inline comments to D136925: [clangd] Index unscoped enums inside classes for code completion.
Oct 30 2022, 12:53 AM · Restricted Project, Restricted Project

Oct 29 2022

nridge added a comment to D136594: [clangd] Add support for semantic token type "operator".

For reference, earlier work along these lines which never landed: https://reviews.llvm.org/D119077

Oct 29 2022, 5:49 PM · Restricted Project, Restricted Project
nridge added a comment to D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler.

(I frequently forget that arc diff does not update the commit message of an existing revision...)

Oct 29 2022, 5:46 PM · Restricted Project, Restricted Project
nridge retitled D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler from [clangd] Introduce CompileCommandsAdjuster to [clangd] Pass the entire tooling::CompileCommand to CommandMangler.
Oct 29 2022, 5:46 PM · Restricted Project, Restricted Project

Oct 24 2022

nridge added a comment to D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler.

(apologies, I'm also very tardy in getting back to this...)

Oct 24 2022, 1:56 AM · Restricted Project, Restricted Project
nridge updated the diff for D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler.

Address review comments

Oct 24 2022, 1:55 AM · Restricted Project, Restricted Project

Oct 18 2022

nridge accepted D136212: [clangd] consider ~^foo() to target the destructor, not the type.

Thanks for fixing!

Oct 18 2022, 10:38 PM · Restricted Project, Restricted Project

Oct 17 2022

nridge added a comment to D127403: [clangd] Implement semantic token modifier "definition".
Oct 17 2022, 5:19 PM · Restricted Project, Restricted Project
nridge committed rGd5a99bf5e134: [clangd] Update 'using enum' semantic highlighting testcase to use the… (authored by nridge).
[clangd] Update 'using enum' semantic highlighting testcase to use the…
Oct 17 2022, 5:16 PM · Restricted Project, Restricted Project
nridge added a comment to D127403: [clangd] Implement semantic token modifier "definition".

Landed https://reviews.llvm.org/rGc93430bae4fc

Oct 17 2022, 4:58 PM · Restricted Project, Restricted Project
nridge committed rGc93430bae4fc: [clangd] Update testcase for issue 1222 to use the 'definition' modifier (authored by nridge).
[clangd] Update testcase for issue 1222 to use the 'definition' modifier
Oct 17 2022, 4:56 PM · Restricted Project, Restricted Project
nridge added a comment to D127403: [clangd] Implement semantic token modifier "definition".

Landing the fix now

Oct 17 2022, 4:51 PM · Restricted Project, Restricted Project
nridge added a comment to D127403: [clangd] Implement semantic token modifier "definition".

Looks like an error introduced when rebasing the patch: a recently added test case (for https://github.com/clangd/clangd/issues/1222) needs to be updated to reflect the _decl to _def change that this patch makes to all semantic highlighting tests. I'll land a fix.

Oct 17 2022, 3:51 PM · Restricted Project, Restricted Project
nridge added a comment to D127403: [clangd] Implement semantic token modifier "definition".

I went ahead and landed this for you

Oct 17 2022, 2:13 PM · Restricted Project, Restricted Project
nridge committed rG4abc910a42e5: [clangd] Implement semantic token modifier "definition" (authored by ckandeler).
[clangd] Implement semantic token modifier "definition"
Oct 17 2022, 2:13 PM · Restricted Project, Restricted Project
nridge closed D127403: [clangd] Implement semantic token modifier "definition".
Oct 17 2022, 2:13 PM · Restricted Project, Restricted Project

Sep 30 2022

nridge added a comment to D134827: [clangd] Avoid recursion on UnresolvedUsingValueDecl during semantic highlighting.

Thanks for the testcases!

Sep 30 2022, 5:36 PM · Restricted Project, Restricted Project

Sep 28 2022

nridge added a comment to D134827: [clangd] Avoid recursion on UnresolvedUsingValueDecl during semantic highlighting.

Speculative fix for an infinite recursion with no testcase. I'm happy to circle back to this if/when we get a testcase, but I don't think there's any downside to this patch in the meantime.

Sep 28 2022, 1:01 PM · Restricted Project, Restricted Project
nridge updated the summary of D134827: [clangd] Avoid recursion on UnresolvedUsingValueDecl during semantic highlighting.
Sep 28 2022, 1:01 PM · Restricted Project, Restricted Project
nridge requested review of D134827: [clangd] Avoid recursion on UnresolvedUsingValueDecl during semantic highlighting.
Sep 28 2022, 1:00 PM · Restricted Project, Restricted Project

Sep 25 2022

nridge updated the diff for D133757: [clangd] Perform system include extraction inside CommandMangler.

This is mostly just a rebase on top of the update to D133756 to turn
CompileCommandsAdjuster into a unique_function, and addressing a few
minor comments.

Sep 25 2022, 1:37 AM · Restricted Project, Restricted Project
nridge updated the diff for D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler.

Updated to use unique_function rather than inheritance

Sep 25 2022, 1:21 AM · Restricted Project, Restricted Project
nridge added inline comments to D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler.
Sep 25 2022, 12:33 AM · Restricted Project, Restricted Project

Sep 20 2022

nridge accepted D134137: [clangd] Return earlier when snippet is empty.
Sep 20 2022, 1:52 AM · Restricted Project, Restricted Project

Sep 19 2022

nridge added inline comments to D134137: [clangd] Return earlier when snippet is empty.
Sep 19 2022, 1:58 AM · Restricted Project, Restricted Project

Sep 17 2022

nridge accepted D133982: [clangd] Improve inlay hints of things expanded from macros.

Thanks for fixing!

Sep 17 2022, 2:16 AM · Restricted Project, Restricted Project

Sep 15 2022

nridge added a comment to D131853: [clangd] Add doxygen parsing for Hover.

Is there anything I can do as a random member of the public that wants this and knows C++?

Sep 15 2022, 4:58 PM · Restricted Project, Restricted Project, Restricted Project
nridge added inline comments to D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler.
Sep 15 2022, 4:53 PM · Restricted Project, Restricted Project
nridge added a comment to D133757: [clangd] Perform system include extraction inside CommandMangler.

Thanks for the feedback! Yeah I'd be wary of introducing a corner case where the user passes --query-driver, and there is in fact a driver available to query in PATH, but we end up not querying it. Even if the outcome is the same in cases we can think of, it feels like a footgun that's going to bite some user somewhere.

Sep 15 2022, 11:06 AM · Restricted Project, Restricted Project
nridge added a comment to D133757: [clangd] Perform system include extraction inside CommandMangler.

I think the sticking point for just having QueryDriverDatabase run after the entirety of CommandMangler is this check in resolveDriver(), which replaces e.g. gcc with /path/to/clang/bin/gcc, which likely does not actually exist (i.e. QueryDriverDatabase will not be able to query it, when it might have been able to look up gcc in PATH).

Sep 15 2022, 12:13 AM · Restricted Project, Restricted Project

Sep 14 2022

nridge added a comment to D133664: [clangd] Fix hover on symbol introduced by using declaration.

Yeah I do have commit access now, so I'll land this by myself.

I don't think I have the permissions to close the corresponding issue on Github though, so someone else would need to do that.

Sep 14 2022, 11:41 AM · Restricted Project, Restricted Project

Sep 13 2022

nridge added inline comments to D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler.
Sep 13 2022, 1:10 PM · Restricted Project, Restricted Project
nridge added inline comments to D133757: [clangd] Perform system include extraction inside CommandMangler.
Sep 13 2022, 1:26 AM · Restricted Project, Restricted Project
nridge added a comment to D133757: [clangd] Perform system include extraction inside CommandMangler.

So far I've only written a test for https://github.com/clangd/clangd/issues/1089.

Sep 13 2022, 1:24 AM · Restricted Project, Restricted Project
nridge added a comment to D133757: [clangd] Perform system include extraction inside CommandMangler.

Depends on https://reviews.llvm.org/D133756

Sep 13 2022, 1:20 AM · Restricted Project, Restricted Project
nridge added a comment to D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler.

This is a refactoring in preparation for D133757, please see that for motivation

Sep 13 2022, 1:19 AM · Restricted Project, Restricted Project
nridge requested review of D133757: [clangd] Perform system include extraction inside CommandMangler.
Sep 13 2022, 1:18 AM · Restricted Project, Restricted Project
nridge requested review of D133756: [clangd] Pass the entire tooling::CompileCommand to CommandMangler.
Sep 13 2022, 1:18 AM · Restricted Project, Restricted Project

Sep 11 2022

Herald added a project to D66254: Correct include suggestion when search path includes symlink: Restricted Project.
Sep 11 2022, 8:38 PM · Restricted Project, Restricted Project

Sep 5 2022

nridge committed rGc93345385830: Fix build error in StmtPrinterTest.cpp (authored by nridge).
Fix build error in StmtPrinterTest.cpp
Sep 5 2022, 12:49 AM · Restricted Project, Restricted Project
nridge committed rG898c421975ed: [clangd] Avoid crash when printing call to string literal operator template (authored by nridge).
[clangd] Avoid crash when printing call to string literal operator template
Sep 5 2022, 12:42 AM · Restricted Project, Restricted Project
nridge closed D132830: [clangd] Avoid crash when printing call to string literal operator template in hover.
Sep 5 2022, 12:42 AM · Restricted Project, Restricted Project, Restricted Project
nridge updated the diff for D132830: [clangd] Avoid crash when printing call to string literal operator template in hover.

Move test to StmtPrinterTests, add test for literal operator using pack

Sep 5 2022, 12:26 AM · Restricted Project, Restricted Project, Restricted Project

Aug 29 2022

nridge committed rG9af0a142e436: [clangd] Fail more gracefully if QueryDriverDatabase cannot determine file type (authored by nridge).
[clangd] Fail more gracefully if QueryDriverDatabase cannot determine file type
Aug 29 2022, 9:59 AM · Restricted Project, Restricted Project
nridge closed D132833: [clangd] Fail more gracefully if QueryDriverDatabase cannot determine file type.
Aug 29 2022, 9:59 AM · Restricted Project, Restricted Project
nridge requested review of D132833: [clangd] Fail more gracefully if QueryDriverDatabase cannot determine file type.
Aug 29 2022, 1:27 AM · Restricted Project, Restricted Project
nridge requested review of D132830: [clangd] Avoid crash when printing call to string literal operator template in hover.
Aug 29 2022, 1:20 AM · Restricted Project, Restricted Project, Restricted Project

Aug 13 2022

nridge added a comment to D131385: [clangd] Support for standard type hierarchy.

Related issue: https://github.com/clangd/clangd/issues/826

Aug 13 2022, 3:11 PM · Restricted Project, Restricted Project

Aug 4 2022

nridge added a comment to D130015: [clangd] Add "usedAsMutablePointer" highlighting modifier.

Sorry, I'm travelling and probably won't be able to respond promptly, but my first thought here is that this doesn't seem as well motivated as usedAsMutableReference, since taking a pointer to an object involves using explicit syntax (&obj) that taking a reference does not. My mental model of usedAsMutableReference is that it's a workaround for the fact that C++ does not have explicit syntax for forming a mutable reference (compare Rust's ref obj which in my mind makes this modifier likewise unnecessary there).

Aug 4 2022, 2:56 PM · Restricted Project, Restricted Project