Page MenuHomePhabricator

sammccall (Sam McCall)
UserAdministrator

Projects

User does not belong to any projects.

User Details

User Since
Aug 26 2016, 6:53 AM (216 w, 6 d)
Roles
Administrator

Recent Activity

Today

sammccall requested review of D90030: [CMake] Fix hardcoding of protobuf output basename. NFC.
Fri, Oct 23, 5:06 AM · Restricted Project
sammccall requested review of D90027: [CMake] generate_grpc_protos -> generate_protos(... GRPC). NFC.
Fri, Oct 23, 4:50 AM · Restricted Project, Restricted Project
sammccall requested review of D90023: [Syntax] Add iterators over children of syntax trees..
Fri, Oct 23, 4:09 AM · Restricted Project
sammccall added inline comments to D89946: [clang] Suppress "follow-up" diagnostics on recovery call expressions..
Fri, Oct 23, 1:30 AM · Restricted Project
sammccall added inline comments to D88553: [clangd] Start using SyntaxTrees for folding ranges feature.
Fri, Oct 23, 1:00 AM · Restricted Project
sammccall added inline comments to D89785: [clangd] Add basic support for attributes (selection, hover).
Fri, Oct 23, 12:33 AM · Restricted Project
sammccall updated the diff for D89785: [clangd] Add basic support for attributes (selection, hover).

Add comment+assert to clarify

Fri, Oct 23, 12:32 AM · Restricted Project

Yesterday

sammccall added inline comments to D89882: [clangd] Migrate to proto2 syntax.
Thu, Oct 22, 4:50 PM · Restricted Project
sammccall updated the diff for D89743: Support Attr in DynTypedNode and ASTMatchers..

Fix nullptr condition (probably dead, but...)

Thu, Oct 22, 2:47 PM · Restricted Project
sammccall added a comment to D89743: Support Attr in DynTypedNode and ASTMatchers..

This is *awesome*, thank you so much for working on it!

Thanks for the comments - haven't addressed them yet, but wanted to clarify scope first.

One question I have is: as it stands, this is not a particularly useful matcher because it can only be used to say "yup, there's an attribute"

Yes definitely, I should have mentioned this...

My main goal here was to support it in DynTypedNode, as we have APIs (clangd::SelectionTree) that can only handle nodes that fit there.
But the common test pattern in ASTTypeTraitsTest used matchers, and I figured a basic matcher wasn't hard to add.

Okay, that makes sense to me.

are you planning to extend this functionality so that you can do something like attr(hasName("foo")), or specify the syntax the attribute uses, isImplicit(), etc?

I hadn't planned to - it definitely makes sense though I don't have any immediate use cases. I can do any of:

  • leave as-is, waiting for someone to add matchers to make it useful
  • scope down the patch to exclude the matcher (and write the ASTTypeTraitsTest another way)
  • add some simple matcher like hasName (I guess in a followup patch) to make it minimally useful, with space for more

What do you think?

My preference is to add support for hasName() as that's going to be the most common need for matching attributes.

Sounds good (though I ran into issues calling this hasName specifically, see below). There's overlap with hasAttr(Kind) but I think that only handles decls, you can't bind the attribute itself, and it's harder to extend to the other stuff you mentioned.
So maybe hasAttr gets deprecated, or maybe has(attr(hasName("foo"))) is a mouthful for a common case so the shorthand is nice.

Thu, Oct 22, 2:44 PM · Restricted Project
sammccall updated the diff for D89743: Support Attr in DynTypedNode and ASTMatchers..

Address review comments.
Add hasAttrName() and make isImplicit() support Attr too.

Thu, Oct 22, 2:43 PM · Restricted Project
sammccall requested changes to D89862: [clangd] Give the server information about client's remote index protocol version.

Add package versioning and make current version v1.

Thu, Oct 22, 12:33 PM · Restricted Project
sammccall updated the diff for D89785: [clangd] Add basic support for attributes (selection, hover).

Only deoptimize selection for *explicit* attributes

Thu, Oct 22, 7:00 AM · Restricted Project
sammccall accepted D89862: [clangd] Give the server information about client's remote index protocol version.

Regarding versioning of grpc layer. In addition to including a version number in every request, looks like there's the concept of "versioned-services".

So we basically change the package name to be versioned, i.e. package clang.clangd.remote.v1 and every time we make a breaking change, we increment the version number and start a new service (while keeping the old one).
Hopefully most of the core pieces will be re-usable, hence this will likely only end up adding a new service definition with possibly new reply/request types.

That might be more manageable than having a version in every request. It will also make handling a little bit easier, as dispatch will happen in grpc layer and server wouldn't have to perform conditional checks.

Good point! @sammccall should we do the package versioning and update to mitigate potential breaking changes?

Thu, Oct 22, 6:31 AM · Restricted Project
sammccall added a comment to D89946: [clang] Suppress "follow-up" diagnostics on recovery call expressions..

Cool, this makes sense to me. I hadn't realized this doesn't go through typoexpr, that's a bit unfortunate we can't address this just in one place.

Thu, Oct 22, 4:52 AM · Restricted Project
sammccall added a comment to D89862: [clangd] Give the server information about client's remote index protocol version.

IME 1 is the most maintainable way to handle minor changes, while 3 is the most maintainable way to handle dramatic changes.

Thu, Oct 22, 3:06 AM · Restricted Project
sammccall added a comment to D89862: [clangd] Give the server information about client's remote index protocol version.

What's the goal here?

Thu, Oct 22, 3:04 AM · Restricted Project

Tue, Oct 20

sammccall added inline comments to D88553: [clangd] Start using SyntaxTrees for folding ranges feature.
Tue, Oct 20, 12:35 PM · Restricted Project
sammccall accepted D89579: [clangd][ObjC] Support nullability annotations.
Tue, Oct 20, 8:31 AM · Restricted Project
sammccall added a comment to D89743: Support Attr in DynTypedNode and ASTMatchers..

This is *awesome*, thank you so much for working on it!

Thanks for the comments - haven't addressed them yet, but wanted to clarify scope first.

Tue, Oct 20, 8:30 AM · Restricted Project
sammccall added inline comments to D88553: [clangd] Start using SyntaxTrees for folding ranges feature.
Tue, Oct 20, 5:48 AM · Restricted Project
sammccall added a comment to D89579: [clangd][ObjC] Support nullability annotations.

Yep, let's revert to the previous state and land that, and I'll puzzle over the examples you give (because always returning false shouldn't affect behavior, just performance).

Tue, Oct 20, 4:26 AM · Restricted Project
sammccall accepted D89783: [format] foo.<name>.h should be the main-header for foo.<name>.cc.

Thank you!

Tue, Oct 20, 4:15 AM · Restricted Project
sammccall updated the diff for D89743: Support Attr in DynTypedNode and ASTMatchers..

Update docs, dynamic registry and tests.

Tue, Oct 20, 3:40 AM · Restricted Project
sammccall requested review of D89785: [clangd] Add basic support for attributes (selection, hover).
Tue, Oct 20, 3:02 AM · Restricted Project

Mon, Oct 19

sammccall requested review of D89743: Support Attr in DynTypedNode and ASTMatchers..
Mon, Oct 19, 2:37 PM · Restricted Project
sammccall updated subscribers of D86936: [clang] Limit the maximum level of fold-expr expansion..

Why did we select the 'bracket-depth' for this? The documentation on that doesn't seem to be anything close to what this should be based on:

Based on a discussion with @rsmith - the formal definition of pack expansion is that it expands to nested parenthesized expressions expressions.

Mon, Oct 19, 7:57 AM · Restricted Project
sammccall added inline comments to D89579: [clangd][ObjC] Support nullability annotations.
Mon, Oct 19, 7:48 AM · Restricted Project
sammccall committed rGcf814fcd3939: [clangd] Add test for structured-binding completion. NFC (authored by sammccall).
[clangd] Add test for structured-binding completion. NFC
Mon, Oct 19, 7:46 AM
sammccall requested review of D89700: [clangd] Don't offer to expand auto in structured binding declarations..
Mon, Oct 19, 7:29 AM · Restricted Project
sammccall committed rG375f7a416031: [ADT] Avoid use of result_of_t in function_ref (authored by sammccall).
[ADT] Avoid use of result_of_t in function_ref
Mon, Oct 19, 6:00 AM
sammccall added a comment to D88977: Reapply [ADT] function_ref's constructor is unavailable if the argument is not callable..

375f7a416031b3a011a5a88ba5f80f5879d775ca should fix this.

Mon, Oct 19, 6:00 AM · Restricted Project
sammccall accepted D89685: [clangd] Rename edge name for filesymbols to slabs in memorytree.
Mon, Oct 19, 5:07 AM · Restricted Project
sammccall added inline comments to D89579: [clangd][ObjC] Support nullability annotations.
Mon, Oct 19, 5:04 AM · Restricted Project
sammccall added a comment to D88977: Reapply [ADT] function_ref's constructor is unavailable if the argument is not callable..

Sorry about the delay.
This is a supported configuration: apple clang toolchain (including libc++) is supposed to work back to 6.0. I'll get this fixed today.

Mon, Oct 19, 2:45 AM · Restricted Project
sammccall updated the diff for D89571: [clangd] Add textDocument/ast extension method to dump the AST.

(for real this time)

Mon, Oct 19, 2:33 AM · Restricted Project
sammccall updated the diff for D89571: [clangd] Add textDocument/ast extension method to dump the AST.

Oops, forgot newly added files :-\

Mon, Oct 19, 2:32 AM · Restricted Project

Fri, Oct 16

sammccall requested review of D89571: [clangd] Add textDocument/ast extension method to dump the AST.
Fri, Oct 16, 11:05 AM · Restricted Project
sammccall accepted D89277: [clangd] Add $/dumpMemoryTree LSP extension.
Fri, Oct 16, 8:27 AM · Restricted Project
sammccall added a comment to D89277: [clangd] Add $/dumpMemoryTree LSP extension.

This looks really nice!

Fri, Oct 16, 2:28 AM · Restricted Project
sammccall accepted D89496: [Format/ObjC] Correctly handle base class with lightweight generics and protocol.
Fri, Oct 16, 2:07 AM · Restricted Project

Wed, Oct 14

sammccall accepted D89425: [Format/ObjC] Add NS_SWIFT_NAME() and CF_SWIFT_NAME() to WhitespaceSensitiveMacros.
Wed, Oct 14, 2:35 PM · Restricted Project
sammccall added a comment to D89277: [clangd] Add $/dumpMemoryTree LSP extension.

(sorry out today and haven't looked at code yet)

Wed, Oct 14, 5:39 AM · Restricted Project

Mon, Oct 12

sammccall added a comment to D88299: [clang-format] Add MacroUnexpander..

This is magnificently difficult :-)
I've sunk a fair few hours into understanding it now, and need to send some comments based on the interface+tests.
Some of these will be misguided or answered by the implementation, but I'm afraid I can't fit it all into my head (on a reasonable timeframe) until I understand it better.

Mon, Oct 12, 11:23 AM · Restricted Project
sammccall resigned from D87170: [json] Provide a means to delegate writing a value to another API.
Mon, Oct 12, 8:45 AM · Restricted Project
sammccall requested review of D89238: [clangd] Go-to-definition from non-renaming alias is unambiguous..
Mon, Oct 12, 7:23 AM · Restricted Project
sammccall accepted D88415: [clangd] Introduce memory usage dumping to TUScheduler, for Preambles and ASTCache.
Mon, Oct 12, 6:16 AM · Restricted Project
sammccall accepted D88417: [clangd] Record memory usages after each notification.
Mon, Oct 12, 6:13 AM · Restricted Project
sammccall accepted D89233: [clangd] Refine recoveryAST flags in clangd.

This is fine as is, but could consider retiring the flags instead.

Mon, Oct 12, 4:57 AM · Restricted Project
sammccall accepted D88413: [clangd] Add a metric for tracking memory usage.
Mon, Oct 12, 4:51 AM · Restricted Project
sammccall added a comment to D89131: [clangd] Validate optional fields more strictly..

Sorry, I thought this was accepted. Had addressed the two comments, happy to address more or revert, LMK

Mon, Oct 12, 4:50 AM · Restricted Project
sammccall committed rG8f1de22c7681: [clangd] Stop capturing trace args if the tracer doesn't need them. (authored by sammccall).
[clangd] Stop capturing trace args if the tracer doesn't need them.
Mon, Oct 12, 4:44 AM
sammccall closed D89135: [clangd] Stop capturing trace args if the tracer doesn't need them..
Mon, Oct 12, 4:44 AM · Restricted Project
sammccall updated the diff for D89135: [clangd] Stop capturing trace args if the tracer doesn't need them..

address comments

Mon, Oct 12, 4:23 AM · Restricted Project
sammccall added inline comments to D89135: [clangd] Stop capturing trace args if the tracer doesn't need them..
Mon, Oct 12, 4:23 AM · Restricted Project
sammccall committed rGc2d4280328e4: [clangd] Validate optional fields more strictly. (authored by sammccall).
[clangd] Validate optional fields more strictly.
Mon, Oct 12, 4:02 AM
sammccall closed D89131: [clangd] Validate optional fields more strictly..
Mon, Oct 12, 4:02 AM · Restricted Project
sammccall committed rG31a575bbc0fc: [JSON] Add ObjectMapper::mapOptional to validate optional data. (authored by sammccall).
[JSON] Add ObjectMapper::mapOptional to validate optional data.
Mon, Oct 12, 3:59 AM
sammccall closed D89128: [JSON] Add ObjectMapper::mapOptional to validate optional data..
Mon, Oct 12, 3:58 AM · Restricted Project
sammccall accepted D88463: [clangd] Try harder to get accurate ranges for documentSymbols in macros.

Thanks! This seems totally complete now, though I bet there's another case possible somehow!

Mon, Oct 12, 2:57 AM · Restricted Project

Fri, Oct 9

sammccall requested review of D89135: [clangd] Stop capturing trace args if the tracer doesn't need them..
Fri, Oct 9, 8:22 AM · Restricted Project
sammccall committed rG41d2987c7558: [clangd] Stop logging in fromJSON, report instead. (authored by sammccall).
[clangd] Stop logging in fromJSON, report instead.
Fri, Oct 9, 7:16 AM
sammccall requested review of D89131: [clangd] Validate optional fields more strictly..
Fri, Oct 9, 7:06 AM · Restricted Project
sammccall requested review of D89128: [JSON] Add ObjectMapper::mapOptional to validate optional data..
Fri, Oct 9, 6:34 AM · Restricted Project
sammccall requested review of D89126: [clangd] Support CodeActionParams.only.
Fri, Oct 9, 6:17 AM · Restricted Project
sammccall committed rG7530b254e93a: [clangd] Make the tweak filter a parameter to enumerateTweaks. NFC (authored by sammccall).
[clangd] Make the tweak filter a parameter to enumerateTweaks. NFC
Fri, Oct 9, 5:19 AM
sammccall closed D88724: [clangd] Make the tweak filter a parameter to enumerateTweaks. NFC.
Fri, Oct 9, 5:19 AM · Restricted Project
sammccall added a comment to D88204: [clangd] Drop path suffix mapping for std symbols.

Taking another look at the header list, there are a couple of classes of symbols beyond c/c++ standard library.

Fri, Oct 9, 4:53 AM · Restricted Project
sammccall added inline comments to D89046: [AST] Build recovery expression by default for all language..
Fri, Oct 9, 4:24 AM · Restricted Project
sammccall accepted D89045: [AST][RecoveryExpr] Don't perform early typo correction in C..
Fri, Oct 9, 4:10 AM · Restricted Project
sammccall accepted D84304: [AST][RecoveryExpr] Part 2: Build dependent callexpr in C for error-recovery..
Fri, Oct 9, 4:09 AM · Restricted Project
sammccall added a comment to D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex.

Now there's lots of usage (which looks good!) i'm finding it a bit hard to keep track of what the tree will look like overall.

At some point it'd be great to:
a) bind this to an LSP extension so we can see it in editors

i was also thinking about it and couldn't decide between a "custom command" vs "code action".

  • the former gives a richer interaction, but requires every editor plugin to implement support.
  • the latter is a little bit more restrictive but doesn't require a bunch of extra work.

I am happy to go with the "code action" approach initially. WDYT? (not in the scope of this patch)

I'm pretty leery about code action because it's not at all context-sensitive (not even per-file).

Another slighty silly reason: because of layering, a code action is going to have a hard time getting at ClangdLSPServer's profile (or even ClangdServer's).
Whereas a custom method will be implemented at that layer.

Fri, Oct 9, 4:05 AM · Restricted Project
sammccall added inline comments to D88417: [clangd] Record memory usages after each notification.
Fri, Oct 9, 4:01 AM · Restricted Project
sammccall added inline comments to D88413: [clangd] Add a metric for tracking memory usage.
Fri, Oct 9, 3:52 AM · Restricted Project
sammccall added inline comments to D88885: [clangd] Disambiguate overloads of std::move for header insertion..
Fri, Oct 9, 1:27 AM · Restricted Project
sammccall committed rG6ee47f552ba7: [clangd] Fix dead variable, typo. NFC (authored by sammccall).
[clangd] Fix dead variable, typo. NFC
Fri, Oct 9, 1:27 AM
sammccall accepted D88814: [clangd] Enable partial namespace matches for workspace symbols.

Yeah, we can fairly easily make this tighter if it's noisy. My suspicion is people will rather complain about ranking than these results being totally irrelevant.
Nice that we now have workspace/symbols users that can send this feedback!

Fri, Oct 9, 1:17 AM · Restricted Project
sammccall added a comment to D89098: [clang] Fix returning the underlying VarDecl as top-level decl for VarTemplateDecl..

Is this https://github.com/clangd/clangd/issues/554 ? :-)

Fri, Oct 9, 1:09 AM · Restricted Project
sammccall accepted D89098: [clang] Fix returning the underlying VarDecl as top-level decl for VarTemplateDecl..

Nice catch, LG with readability nits

Fri, Oct 9, 1:09 AM · Restricted Project
sammccall added a comment to D88417: [clangd] Record memory usages after each notification.

Bad news, I was testing this with remote-index, hence background-index was turned off. Unfortunately traversing all of the slabs in FileSymbols takes quite a while in this case (~15ms for LLVM).

I don't think it is feasible to do this on every notification now, as this implies an extra 15ms latency for interactive requests like code completion/signature help due to the delay between didChange notification and codeCompletion request.

Fri, Oct 9, 12:55 AM · Restricted Project
sammccall added a comment to D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex.

Now there's lots of usage (which looks good!) i'm finding it a bit hard to keep track of what the tree will look like overall.

At some point it'd be great to:
a) bind this to an LSP extension so we can see it in editors

i was also thinking about it and couldn't decide between a "custom command" vs "code action".

  • the former gives a richer interaction, but requires every editor plugin to implement support.
  • the latter is a little bit more restrictive but doesn't require a bunch of extra work.

I am happy to go with the "code action" approach initially. WDYT? (not in the scope of this patch)

Fri, Oct 9, 12:13 AM · Restricted Project

Wed, Oct 7

sammccall committed rG69daa368cad3: [clangd] Disambiguate overloads of std::move for header insertion. (authored by sammccall).
[clangd] Disambiguate overloads of std::move for header insertion.
Wed, Oct 7, 10:42 AM
sammccall closed D88885: [clangd] Disambiguate overloads of std::move for header insertion..
Wed, Oct 7, 10:42 AM · Restricted Project
sammccall updated the diff for D88885: [clangd] Disambiguate overloads of std::move for header insertion..

Refactor the hack into a more appropriate place.

Wed, Oct 7, 10:36 AM · Restricted Project
sammccall added a comment to D88885: [clangd] Disambiguate overloads of std::move for header insertion..

Thanks, LGTM! As you mentioned I believe std::move is common enough to deserve the special casing.

Wed, Oct 7, 10:36 AM · Restricted Project
sammccall added inline comments to D88814: [clangd] Enable partial namespace matches for workspace symbols.
Wed, Oct 7, 10:23 AM · Restricted Project
sammccall accepted D85354: [clangd] Reduce availability of extract function.
Wed, Oct 7, 9:58 AM · Restricted Project
sammccall accepted D84387: [AST][RecoveryExpr] Part4: Support dependent cast-expr in C for error-recovery..
Wed, Oct 7, 9:56 AM · Restricted Project
sammccall added a comment to D88417: [clangd] Record memory usages after each notification.

this is at least going to take some important locks, hopefully only briefly.

Wed, Oct 7, 9:54 AM · Restricted Project
sammccall accepted D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex.

Now there's lots of usage (which looks good!) i'm finding it a bit hard to keep track of what the tree will look like overall.

Wed, Oct 7, 9:51 AM · Restricted Project
sammccall accepted D88417: [clangd] Record memory usages after each notification.
Wed, Oct 7, 9:41 AM · Restricted Project
sammccall accepted D88411: [clangd] Introduce MemoryTrees.

Still LGTM, commit it already :-D

Wed, Oct 7, 9:37 AM · Restricted Project
sammccall committed rG91a98ec11e2e: [json] Provide a means to delegate writing a value to another API (authored by dsanders).
[json] Provide a means to delegate writing a value to another API
Wed, Oct 7, 9:36 AM
sammccall closed D88902: [json] Provide a means to delegate writing a value to another API.
Wed, Oct 7, 9:35 AM · Restricted Project
sammccall added a reverting change for rG281703e67ffa: Revert "[ADT] function_ref's constructor is unavailable if the argument is not…: rGb953a01b2cd0: Reapply [ADT] function_ref's constructor is unavailable if the argument is not….
Wed, Oct 7, 9:31 AM
sammccall committed rGb953a01b2cd0: Reapply [ADT] function_ref's constructor is unavailable if the argument is not… (authored by sammccall).
Reapply [ADT] function_ref's constructor is unavailable if the argument is not…
Wed, Oct 7, 9:31 AM
sammccall closed D88977: Reapply [ADT] function_ref's constructor is unavailable if the argument is not callable..
Wed, Oct 7, 9:31 AM · Restricted Project
sammccall added a comment to D88977: Reapply [ADT] function_ref's constructor is unavailable if the argument is not callable..

If you have some pointers to gcc bug being discussed in some place, it might be nice to include.

Wed, Oct 7, 9:31 AM · Restricted Project
sammccall added a reverting change for rG281703e67ffa: Revert "[ADT] function_ref's constructor is unavailable if the argument is not…: D88977: Reapply [ADT] function_ref's constructor is unavailable if the argument is not callable..
Wed, Oct 7, 9:11 AM
sammccall requested review of D88977: Reapply [ADT] function_ref's constructor is unavailable if the argument is not callable..
Wed, Oct 7, 9:11 AM · Restricted Project