Page MenuHomePhabricator

sammccall (Sam McCall)
UserAdministrator

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Yesterday

sammccall added inline comments to D87891: [clangd] findNearbyIdentifier(): guaranteed to give up after 2^N lines.
Wed, Sep 23, 11:53 PM · Restricted Project
sammccall committed rG2bd5e3fb3cc0: [clangd] Improve bad-RPC-payload error messages slightly (authored by sammccall).
[clangd] Improve bad-RPC-payload error messages slightly
Wed, Sep 23, 4:52 PM
sammccall committed rG751f5c814689: Fix LLDB tweak in 62a47e994fcf5b73e29547d26cd9676b30cb69a3 (authored by sammccall).
Fix LLDB tweak in 62a47e994fcf5b73e29547d26cd9676b30cb69a3
Wed, Sep 23, 4:31 PM
sammccall closed D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..
Wed, Sep 23, 4:28 PM · Restricted Project, Restricted Project, Restricted Project
sammccall updated the diff for D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..

This was landed as 4 commits, this diff is all 4 as committed.

Wed, Sep 23, 4:28 PM · Restricted Project, Restricted Project, Restricted Project
sammccall reopened D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..
Wed, Sep 23, 4:26 PM · Restricted Project, Restricted Project, Restricted Project
sammccall committed rGfa69b608063e: [JSON] Add error reporting to fromJSON and ObjectMapper (authored by sammccall).
[JSON] Add error reporting to fromJSON and ObjectMapper
Wed, Sep 23, 4:21 PM
sammccall closed D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..
Wed, Sep 23, 4:20 PM · Restricted Project, Restricted Project, Restricted Project
sammccall reopened D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..
Wed, Sep 23, 3:35 PM · Restricted Project, Restricted Project, Restricted Project
sammccall committed rG38de1c33a837: [JSON] Display errors associated with Paths in context (authored by sammccall).
[JSON] Display errors associated with Paths in context
Wed, Sep 23, 3:34 PM
sammccall closed D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..
Wed, Sep 23, 3:34 PM · Restricted Project, Restricted Project, Restricted Project
sammccall reopened D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..
Wed, Sep 23, 3:34 PM · Restricted Project, Restricted Project, Restricted Project
sammccall committed rG16619e7139bd: [JSON] Facility to track position within an object and report errors. (authored by sammccall).
[JSON] Facility to track position within an object and report errors.
Wed, Sep 23, 3:10 PM
sammccall closed D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..
Wed, Sep 23, 3:10 PM · Restricted Project, Restricted Project, Restricted Project
sammccall reopened D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..
Wed, Sep 23, 2:35 PM · Restricted Project, Restricted Project, Restricted Project
sammccall committed rG140b7b6f09ca: [JSON] Allow emitting comments in json::OStream (authored by sammccall).
[JSON] Allow emitting comments in json::OStream
Wed, Sep 23, 2:35 PM
sammccall closed D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..
Wed, Sep 23, 2:35 PM · Restricted Project, Restricted Project, Restricted Project
sammccall updated the diff for D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..

Address review comments.

Wed, Sep 23, 2:17 PM · Restricted Project, Restricted Project, Restricted Project
sammccall added a comment to D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..

Thanks! I've addressed most of the comments one way or the other, please LMK if it's not convincing!
Meanwhile I'm going to split this up to land in parts.

Wed, Sep 23, 2:16 PM · Restricted Project, Restricted Project, Restricted Project
sammccall added a comment to D87673: [clangd] Don't use zlib when it's unavailable..

Thanks, this seems better than crashing.
The practical result isn't wonderful: the two are going to fight over index files to some extent. But this can happen with different clangd versions (that use different index format versions) anyway. So this is really just graceful recovery from a bad situation.

Unsure about test for this

Agree, testing this seems hard and not that useful.

FWIW here's how to test codepaths only reachable when zlib is not available:

./llvm/test/tools/llvm-dwp/X86/nocompress.test:

REQUIRES: !zlib
Wed, Sep 23, 12:41 PM · Restricted Project
sammccall accepted D88144: [clangd] Disable suffix matching fallback for C during include insertion.

(I do wonder whether it's safe to just drop the mapping table entirely now...)

Wed, Sep 23, 11:47 AM · Restricted Project
sammccall added a comment to D88172: [clangd] Extract common file-caching logic from ConfigProvider..

To give a bit of context here: the end-goal here is hot-reload of compile_commands.json. (This covers our *two* top bugs: #192 and #83).

Wed, Sep 23, 11:45 AM · Restricted Project
sammccall requested review of D88172: [clangd] Extract common file-caching logic from ConfigProvider..
Wed, Sep 23, 11:01 AM · Restricted Project
sammccall accepted D79500: [clangd] Refactor code completion signal's utility properties..
Wed, Sep 23, 6:04 AM · Restricted Project

Tue, Sep 22

sammccall accepted D83296: [clang-format] Add a MacroExpander..

Ship it!

Tue, Sep 22, 11:33 AM · Restricted Project, Restricted Project
sammccall committed rG625761825620: [ASTMatchers] Avoid recursion in ancestor matching to save stack space. (authored by sammccall).
[ASTMatchers] Avoid recursion in ancestor matching to save stack space.
Tue, Sep 22, 10:44 AM
sammccall closed D86964: [ASTMatchers] Avoid recursion in ancestor matching to save stack space..
Tue, Sep 22, 10:44 AM · Restricted Project
sammccall added inline comments to D86964: [ASTMatchers] Avoid recursion in ancestor matching to save stack space..
Tue, Sep 22, 10:28 AM · Restricted Project
sammccall added a comment to D87335: [json] Create some llvm::Expected-based accessors.

A rough design sketch

Tue, Sep 22, 9:21 AM · Restricted Project
sammccall updated the summary of D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..
Tue, Sep 22, 9:19 AM · Restricted Project, Restricted Project, Restricted Project
sammccall requested review of D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper..
Tue, Sep 22, 9:18 AM · Restricted Project, Restricted Project, Restricted Project

Mon, Sep 21

sammccall accepted D87080: [AST] Reduce the size of TemplateArgumentLocInfo..

Forgot to mention, 3% memory saving is huge, way more than I expected (was mostly just hoping for no regression).
Nice work!

Mon, Sep 21, 2:46 AM · Restricted Project
sammccall added a comment to D87335: [json] Create some llvm::Expected-based accessors.

A rough design sketch:

Mon, Sep 21, 2:41 AM · Restricted Project
sammccall added a comment to D87335: [json] Create some llvm::Expected-based accessors.

TL;DR:

  • I agree we should solve this, thanks for pushing back
  • I think the path-to-error (e.g. processes[0].triple) is critical in some cases
  • this really seems like an marshaling concern rather than something for Value/Object
  • happy to take a stab at a design but I think it'll need some prototyping
Mon, Sep 21, 2:15 AM · Restricted Project
sammccall added inline comments to D87080: [AST] Reduce the size of TemplateArgumentLocInfo..
Mon, Sep 21, 12:52 AM · Restricted Project
sammccall added inline comments to D87350: [AST][RecoveryExpr] Fix a crash on undeduced type..
Mon, Sep 21, 12:30 AM · Restricted Project

Fri, Sep 18

sammccall added a comment to D87335: [json] Create some llvm::Expected-based accessors.

Argh, I hit enter too soon. One thing I'd planned to add was I'm sorry about the delay in reviewing this, too.

Fri, Sep 18, 9:08 AM · Restricted Project
sammccall added a comment to D87335: [json] Create some llvm::Expected-based accessors.

This is a sensible thing to want and very nice code.
I'm not sure about putting it in JSON.h - there are a few unfortunate aspects that I just don't know how to fix, but limit the benefit which has to be measured against the cost of adding more complexity to the API.

Fri, Sep 18, 9:03 AM · Restricted Project
sammccall accepted D87382: [AST] Fix dependence-bits for CXXDefaultInitExpr..
Fri, Sep 18, 7:56 AM · Restricted Project
sammccall accepted D85611: Improve dynamic AST matching diagnostics for conversion errors.

Sorry about the delay, good guess as to what was happening :-)

Fri, Sep 18, 7:47 AM
sammccall added a comment to D87080: [AST] Reduce the size of TemplateArgumentLocInfo..

Looks good apart from a minor technical issue with the traits.

Fri, Sep 18, 4:30 AM · Restricted Project
sammccall added a comment to D87170: [json] Provide a means to delegate writing a value to another API.

This is really nice. There's an argument for parsing and emitting (validation and formatting) but obviously also a performance tradeoff so the feature makes sense.

Fri, Sep 18, 4:17 AM · Restricted Project
sammccall added a comment to D84387: [AST][RecoveryExpr] Part4: Suppress spurious "err_typecheck_expect_scalar_operand" diagnostic.

I'm not sure I love having the assertion for contains-errors every place that handles dependent code in C.
We should certainly somehow document the reason dependence is possible, just as the possibility of overloads is documented.
But the assertion seems pretty heavyweight :-\

Fri, Sep 18, 3:56 AM · Restricted Project
sammccall added inline comments to D84322: [AST][RecoveryExpr] Part3: Suppress spurious "typecheck_cond_expect_scalar" diagnostic.
Fri, Sep 18, 3:52 AM · Restricted Project
sammccall added inline comments to D84226: [AST][RecoveryExpr] Part1: Support dependent binary operator in C for error recovery..
Fri, Sep 18, 3:50 AM · Restricted Project
sammccall added inline comments to D84304: [AST][RecoveryExpr] Part 2: Build dependent callexpr in C for error-recovery..
Fri, Sep 18, 3:35 AM · Restricted Project
sammccall added inline comments to D84226: [AST][RecoveryExpr] Part1: Support dependent binary operator in C for error recovery..
Fri, Sep 18, 3:21 AM · Restricted Project
sammccall accepted D86765: [clang] Don't emit "no member" diagnostic if the lookup fails on an invalid record decl..

This seems like fixing a fairly specific case of a general problem: if the base class is invalid I would expect to see similar problems in other places (e.g. unqualified lookup from inside the class).
But it fixes a diagnostic the libc++ folks think is important...

Fri, Sep 18, 2:23 AM · Restricted Project, Restricted Project
sammccall added a comment to D83296: [clang-format] Add a MacroExpander..

Just checking this is waiting on you rather than me...

Fri, Sep 18, 2:13 AM · Restricted Project, Restricted Project
sammccall added a comment to D82284: [AST][RecoveryAST] Preseve invalid return stmt, and suppress the diagnostics for missing return stmt in constexpr func..

Any interest in chasing these diagnostic regressions (they seem like they might be tractable) or should we abandon this?

Fri, Sep 18, 2:07 AM · Restricted Project
sammccall resigned from D74731: [Clangd] Fixed assertion when processing extended ASCII characters..
Fri, Sep 18, 2:04 AM · Restricted Project
sammccall resigned from D77507: [clangd] Fix HitMapping assertion in Tokens.cpp.
Fri, Sep 18, 2:04 AM · Restricted Project
sammccall added a comment to D76831: [AST] Preserve the DeclRefExpr when it refers to an invalid decl..

@hokein I think this patch is obsolete, right?

Fri, Sep 18, 1:55 AM · Restricted Project
sammccall resigned from D77527: [AST] Adjust existing tests for recovery-exprs.

(This is obsolete now I believe, the flag has been flipped)

Fri, Sep 18, 1:41 AM · Restricted Project
sammccall closed D72581: [Syntax] Add mapping from spelled to expanded tokens for TokenBuffer.
Fri, Sep 18, 1:24 AM · Restricted Project
sammccall added a comment to D71586: [clangd] Add a test case that verifies -fimplicit-modules work in Clangd when building the AST.

We do now have tests in unittests/ModulesTests.cpp that likely covers at least part of this?

Fri, Sep 18, 1:23 AM · Restricted Project
sammccall added a comment to D80109: [AST][RecoveryExpr] Preserve type for broken member call expr..

@hokein Did this get landed at some point?

Fri, Sep 18, 1:18 AM · Restricted Project
sammccall added a comment to D86077: [clangd] Add a way for exporting memory usage metrics.

Just for the record, as I think Kadir and Adam are both aware...

Fri, Sep 18, 1:17 AM · Restricted Project
sammccall added inline comments to D87350: [AST][RecoveryExpr] Fix a crash on undeduced type..
Fri, Sep 18, 1:13 AM · Restricted Project
sammccall accepted D83814: [clangd] Add Random Forest runtime for code completion..

LG from my side!

Fri, Sep 18, 1:06 AM · Restricted Project
sammccall accepted D87775: [clangd] Add option for disabling AddUsing tweak on some namespaces..

Thanks!
Can we add a simple test to TweakTests (I should really split up that file) and to ConfigCompileTests?

Fri, Sep 18, 1:02 AM · Restricted Project

Wed, Sep 16

sammccall added a comment to D87775: [clangd] Add option for disabling AddUsing tweak on some namespaces..

Nice!

Wed, Sep 16, 10:43 AM · Restricted Project
sammccall added a comment to D83814: [clangd] Add Random Forest runtime for code completion..

Just some naming and doc nits. This looks really solid now, nice job!

Wed, Sep 16, 7:52 AM · Restricted Project
sammccall added inline comments to D87349: [clang] adapt c++17 behavior for dependent decltype-specifiers.
Wed, Sep 16, 7:04 AM · Restricted Project
sammccall added a comment to D87745: [clangd] Config: handle Index block.

Thanks for catching this! Lucky coincidence it was spotted twice just before rc3!

Wed, Sep 16, 6:04 AM · Restricted Project
sammccall added a comment to D87710: [clangd] Actually parse Index section of the YAML file..

Thanks Hans!

Wed, Sep 16, 6:03 AM · Restricted Project
sammccall committed rGf5c7102dbc72: Update dead links to Itanium and ARM ABIs. NFC (authored by sammccall).
Update dead links to Itanium and ARM ABIs. NFC
Wed, Sep 16, 4:42 AM
sammccall added a comment to D87349: [clang] adapt c++17 behavior for dependent decltype-specifiers.

Ugh, the hole goes deeper, because the Itanium ABI unambiguously follows the C++11 (instantiation-dependent) behavior.
Definitely need Richard's input here as I'm fairly sure this will break things as-is.

Wed, Sep 16, 4:37 AM · Restricted Project
sammccall added a comment to D87349: [clang] adapt c++17 behavior for dependent decltype-specifiers.

We should check if the mangling matches GCC (or if it depends on -std), if we can.

Wed, Sep 16, 1:30 AM · Restricted Project
sammccall added a comment to D87686: [clang-tidy] Improve documentation on Clangd integration.

Thanks for doing this! (Agree with Haojian's comments too)

Wed, Sep 16, 1:22 AM · Restricted Project
sammccall accepted D87673: [clangd] Don't use zlib when it's unavailable..

Thanks, this seems better than crashing.
The practical result isn't wonderful: the two are going to fight over index files to some extent. But this can happen with different clangd versions (that use different index format versions) anyway. So this is really just graceful recovery from a bad situation.

Wed, Sep 16, 12:33 AM · Restricted Project
sammccall accepted D87710: [clangd] Actually parse Index section of the YAML file..

Argh, sorry. I think this might be too late to get into 11 - we're in the "soon=final" stage and maybe any changes at all cause logistical problems? :-(

Wed, Sep 16, 12:27 AM · Restricted Project

Mon, Sep 14

sammccall committed rG687e1d712164: [clangd] makeStringError,make_error<StringError> -> error() (authored by sammccall).
[clangd] makeStringError,make_error<StringError> -> error()
Mon, Sep 14, 2:49 AM
sammccall committed rG30667c967d3f: [clangd] Add error() function for creating formatv-style llvm::Errors. NFC (authored by sammccall).
[clangd] Add error() function for creating formatv-style llvm::Errors. NFC
Mon, Sep 14, 1:53 AM
sammccall closed D83419: [clangd] Add error() function for creating formatv-style llvm::Errors. NFC.
Mon, Sep 14, 1:53 AM · Restricted Project

Thu, Sep 3

sammccall added a comment to D87080: [AST] Reduce the size of TemplateArgumentLocInfo..

Nit on the commit message: I think this is TemplateArgumentLoc 48 -> 32 bytes, and DynTypedNode 56 -> 40 bytes.

Thu, Sep 3, 8:58 AM · Restricted Project
sammccall updated subscribers of D86048: [AST][RecoveryExpr] Popagate the error-bit from a VarDecl's initializer to DeclRefExpr..

Neither of the testcases look like the right behavior to me, and I think the bug is elsewhere in clang.

Thu, Sep 3, 7:31 AM · Restricted Project

Wed, Sep 2

sammccall accepted D86936: [clang] Limit the maximum level of fold-expr expansion..
Wed, Sep 2, 6:41 AM · Restricted Project

Tue, Sep 1

sammccall added inline comments to D86936: [clang] Limit the maximum level of fold-expr expansion..
Tue, Sep 1, 1:10 PM · Restricted Project
sammccall requested review of D86964: [ASTMatchers] Avoid recursion in ancestor matching to save stack space..
Tue, Sep 1, 11:24 AM · Restricted Project

Mon, Aug 31

sammccall accepted D83228: [llvm] [unittests] Remove some temporary files after they're not needed.

Sorry, this fell off my radar :-(

Mon, Aug 31, 5:23 AM · Restricted Project

Thu, Aug 27

sammccall added a comment to D86685: [libcxx] Fix the broken test after D82657..

Strangely, -Xclang -fno-recovery-ast-type doesn't make it go away - how sure are we that your patch is the culprit? And how sure are we that those flags work properly?

Thu, Aug 27, 9:22 AM · Restricted Project
sammccall added a comment to D86685: [libcxx] Fix the broken test after D82657..

@hokein here's a minimized version:

Thu, Aug 27, 8:26 AM · Restricted Project
sammccall committed rG266825620c7f: [Tooling][Format] Treat compound extensions (foo.bar.cc) as matching foo.h (authored by sammccall).
[Tooling][Format] Treat compound extensions (foo.bar.cc) as matching foo.h
Thu, Aug 27, 6:35 AM
sammccall closed D86597: [Tooling][Format] Treat compound extensions (foo.bar.cc) as matching foo.h.
Thu, Aug 27, 6:34 AM · Restricted Project
sammccall added inline comments to D86688: [RecoveryExpr] Add 11.0.0 release note..
Thu, Aug 27, 6:18 AM · Restricted Project
sammccall accepted D86688: [RecoveryExpr] Add 11.0.0 release note..
Thu, Aug 27, 6:02 AM · Restricted Project

Wed, Aug 26

sammccall accepted D86624: [clang] Exclude invalid destructors from lookups..
Wed, Aug 26, 7:57 AM · Restricted Project
sammccall accepted D86602: [clangd] Enable recovery-ast-type by default..
Wed, Aug 26, 4:18 AM · Restricted Project
sammccall accepted D86604: [clangd] Use string[] for allCommitCharacters.

Oops, nice catch!

Wed, Aug 26, 4:18 AM · Restricted Project
sammccall requested review of D86597: [Tooling][Format] Treat compound extensions (foo.bar.cc) as matching foo.h.
Wed, Aug 26, 1:33 AM · Restricted Project

Aug 21 2020

sammccall accepted D82657: [AST][RecoveryAST] Preserve the type by default for recovery expression..

New diagnostics are really good :-)

Aug 21 2020, 4:15 AM · Restricted Project
sammccall accepted D85716: [AST][RecoveryExpr] Fix a bogus unused diagnostic when the type is preserved..
Aug 21 2020, 4:08 AM · Restricted Project
sammccall accepted D85635: [clangd] Compute the inactive code range for semantic highlighting..

I think the code could be a bit clearer, but up to you. The tests are good so feel free to submit any version you're happy with.

Aug 21 2020, 3:54 AM · Restricted Project
sammccall accepted D85354: [clangd] Reduce availability of extract function.
Aug 21 2020, 1:47 AM · Restricted Project

Aug 20 2020

sammccall added inline comments to D85923: [clangd] Fix crash-bug in preamble indexing when using modules..
Aug 20 2020, 5:48 AM · Restricted Project

Aug 19 2020

sammccall added inline comments to D83296: [clang-format] Add a MacroExpander..
Aug 19 2020, 10:12 AM · Restricted Project, Restricted Project
sammccall added a comment to D83296: [clang-format] Add a MacroExpander..

Somehow I missed the email from your replies.

Aug 19 2020, 8:37 AM · Restricted Project, Restricted Project
sammccall accepted D86069: [clang] When loading preamble from AST file, re-export modules in Sema..

Sorry, I was just not reading the test clearly. LGTM

Aug 19 2020, 4:27 AM · Restricted Project

Aug 18 2020

sammccall added inline comments to D86069: [clang] When loading preamble from AST file, re-export modules in Sema..
Aug 18 2020, 5:01 AM · Restricted Project