Page MenuHomePhabricator

sammccall (Sam McCall)Administrator
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Yesterday

sammccall accepted D58387: [clangd] Add an option in the code to not display number of fixes.
Tue, Feb 19, 8:19 AM · Restricted Project, Restricted Project
sammccall added a comment to D58345: [clangd] Using symbol name to map includes for STL symbols..

Looks great! Thanks for doing this.

Could you also check in the tool that is used to generate the mapping? We need a way to update the mapping when cppreference is updated.

The tool is not ready yet, I'm still polishing it, but the results are good, I think this patch should not be blocked by the tool.

Tue, Feb 19, 12:11 AM · Restricted Project

Mon, Feb 18

sammccall accepted D58185: [clangd] Handle unresolved scope specifier when fixing includes..

Thanks! The layering is *much* clearer now.
I can still suggest a couple of tweaks, but they're pretty much cosmetic.

Mon, Feb 18, 10:29 AM · Restricted Project, Restricted Project
sammccall added inline comments to D58345: [clangd] Using symbol name to map includes for STL symbols..
Mon, Feb 18, 7:59 AM · Restricted Project
sammccall added a comment to D57400: Add a .gitignore file to the root that ignores any files outside of the project directories..

FWIW, I am convinced by @kristina's arguments. Given that we store build trees in other stuff in the root folder (a practice which I am still trying to digest), I think we should have some safeguards to prevent them from accidentally being comitted. However, I don't feel like the right person to hit "approve" here.

Mon, Feb 18, 5:00 AM
sammccall accepted D58239: [clangd] Cache include fixes for diagnostics caused by the same unresolved name or incomplete type..

SymbolSlab is much cleaner, nice!

Mon, Feb 18, 4:17 AM · Restricted Project, Restricted Project
sammccall added inline comments to D58185: [clangd] Handle unresolved scope specifier when fixing includes..
Mon, Feb 18, 2:32 AM · Restricted Project, Restricted Project

Fri, Feb 15

sammccall updated the diff for D58291: [clangd] Include textual diagnostic ID as Diagnostic.code..

Unit test.

Fri, Feb 15, 10:48 AM · Restricted Project
sammccall created D58291: [clangd] Include textual diagnostic ID as Diagnostic.code..
Fri, Feb 15, 10:43 AM · Restricted Project
sammccall accepted D58062: Support framework import/include auto-completion.

Great, thank you! Want me to land this?

Fri, Feb 15, 9:06 AM · Restricted Project
sammccall added a comment to D58239: [clangd] Cache include fixes for diagnostics caused by the same unresolved name or incomplete type..

(big thumbs up to the caching overall of course!)

Fri, Feb 15, 9:03 AM · Restricted Project, Restricted Project
sammccall added inline comments to D58239: [clangd] Cache include fixes for diagnostics caused by the same unresolved name or incomplete type..
Fri, Feb 15, 9:01 AM · Restricted Project, Restricted Project
sammccall updated the diff for D58275: [clangd] Support utf-8 offsets (rather than utf-16) as a protocol extension.

Add tests, command-line flag, and misc cleanups.
No flag for clangd-indexer yet. It's surprisingly hard with the executor API :-(

Fri, Feb 15, 6:07 AM · Restricted Project
sammccall added a comment to D58275: [clangd] Support utf-8 offsets (rather than utf-16) as a protocol extension.

The code looks good. For this protocol extension, we need supports from other LSP clients, I think we may want to propose this extension to the LSP specification, so that all LSP servers/clients respect it.

Fri, Feb 15, 4:50 AM · Restricted Project
sammccall committed rG0446b40b6369: [clangd] Unlink VFS working dir from OS working dir. Reland of r351051 (authored by sammccall).
[clangd] Unlink VFS working dir from OS working dir. Reland of r351051
Fri, Feb 15, 3:05 AM
sammccall committed rCTE354116: [clangd] Unlink VFS working dir from OS working dir. Reland of r351051.
[clangd] Unlink VFS working dir from OS working dir. Reland of r351051
Fri, Feb 15, 3:04 AM
sammccall committed rL354116: [clangd] Unlink VFS working dir from OS working dir. Reland of r351051.
[clangd] Unlink VFS working dir from OS working dir. Reland of r351051
Fri, Feb 15, 3:04 AM
sammccall created D58275: [clangd] Support utf-8 offsets (rather than utf-16) as a protocol extension.
Fri, Feb 15, 2:19 AM · Restricted Project
sammccall committed rG24f135733dc6: Revert "[Analysis] -Wunreachable-code shouldn't fire on the increment of a… (authored by sammccall).
Revert "[Analysis] -Wunreachable-code shouldn't fire on the increment of a…
Fri, Feb 15, 1:19 AM
sammccall committed rC354109: Revert "[Analysis] -Wunreachable-code shouldn't fire on the increment of a….
Revert "[Analysis] -Wunreachable-code shouldn't fire on the increment of a…
Fri, Feb 15, 1:19 AM
sammccall committed rL354109: Revert "[Analysis] -Wunreachable-code shouldn't fire on the increment of a….
Revert "[Analysis] -Wunreachable-code shouldn't fire on the increment of a…
Fri, Feb 15, 1:19 AM

Thu, Feb 14

sammccall committed rGce2b40def176: [Analysis] -Wunreachable-code shouldn't fire on the increment of a foreach loop (authored by sammccall).
[Analysis] -Wunreachable-code shouldn't fire on the increment of a foreach loop
Thu, Feb 14, 11:16 PM
sammccall committed rC354102: [Analysis] -Wunreachable-code shouldn't fire on the increment of a foreach loop.
[Analysis] -Wunreachable-code shouldn't fire on the increment of a foreach loop
Thu, Feb 14, 11:16 PM
sammccall committed rL354102: [Analysis] -Wunreachable-code shouldn't fire on the increment of a foreach loop.
[Analysis] -Wunreachable-code shouldn't fire on the increment of a foreach loop
Thu, Feb 14, 11:16 PM
sammccall closed D58134: [Analysis] -Wunreachable-code shouldn't fire on the increment of a foreach loop.
Thu, Feb 14, 11:16 PM · Restricted Project, Restricted Project
sammccall committed rG15e475e2220c: Reapply [VFS] Allow multiple RealFileSystem instances with independent CWDs. (authored by sammccall).
Reapply [VFS] Allow multiple RealFileSystem instances with independent CWDs.
Thu, Feb 14, 4:59 AM
sammccall committed rL354026: Reapply [VFS] Allow multiple RealFileSystem instances with independent CWDs..
Reapply [VFS] Allow multiple RealFileSystem instances with independent CWDs.
Thu, Feb 14, 4:59 AM
sammccall closed D58169: Reapply [VFS] Allow multiple RealFileSystem instances with independent CWDs..
Thu, Feb 14, 4:59 AM · Restricted Project

Wed, Feb 13

sammccall updated the diff for D58169: Reapply [VFS] Allow multiple RealFileSystem instances with independent CWDs..

Fix silliness in test helper: only canonicalize unique dir names, don't clobber input.

Wed, Feb 13, 3:29 AM · Restricted Project
sammccall created D58169: Reapply [VFS] Allow multiple RealFileSystem instances with independent CWDs..
Wed, Feb 13, 3:26 AM · Restricted Project
sammccall added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Wed, Feb 13, 2:05 AM · Restricted Project
sammccall added a comment to D58062: Support framework import/include auto-completion.

Code looks good apart from one case where we encounter Foo.framework/Subdir1/Subdir2.

Wed, Feb 13, 1:42 AM · Restricted Project
sammccall added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Wed, Feb 13, 1:09 AM · Restricted Project

Tue, Feb 12

sammccall added inline comments to D56370: [clangd] Add support for type hierarchy (super types only for now).
Tue, Feb 12, 6:55 PM · Restricted Project
sammccall added a comment to D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS.

In the same idea, at the time the plan was that if we had gone with multiple repos instead of a monorepo, there wouldn't have been a separate clang-tools-extra repo.

Tue, Feb 12, 6:02 PM · Restricted Project
sammccall added a comment to D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS.

I think I'm +1 for this change, and to preserving and strengthening the distinction between CTE and Clang.

Tue, Feb 12, 5:52 PM · Restricted Project
sammccall accepted D58135: [clangd] Handle a few more diag kinds in include fixer..

Nice!

Tue, Feb 12, 9:38 AM · Restricted Project, Restricted Project
sammccall created D58134: [Analysis] -Wunreachable-code shouldn't fire on the increment of a foreach loop.
Tue, Feb 12, 9:02 AM · Restricted Project, Restricted Project
sammccall accepted D58126: [clangd] Fix a lit-test..

Hmm, this removes a useful assertion, and still leaves us sending invalid (or at least non-canonical) URIs in the input.
But I don't see a better fix really - lit tests are painful :-(

Tue, Feb 12, 8:21 AM · Restricted Project
sammccall committed rGa860219c5e3f: [LoopSimplifyCFG] Fix test broken in release mode in r353813 (authored by sammccall).
[LoopSimplifyCFG] Fix test broken in release mode in r353813
Tue, Feb 12, 6:44 AM
sammccall added inline comments to D57221: [LoopSimplifyCFG] Change logic of dead loops removal to avoid hitting asserts.
Tue, Feb 12, 6:43 AM · Restricted Project
sammccall committed rL353842: [LoopSimplifyCFG] Fix test broken in release mode in r353813.
[LoopSimplifyCFG] Fix test broken in release mode in r353813
Tue, Feb 12, 6:43 AM
sammccall accepted D58111: [Sema] Fix a crash in access checking for deduction guides.
Tue, Feb 12, 2:58 AM · Restricted Project
sammccall committed rG905438a4b556: [clangd] Fix use-after-free in XRefs (authored by sammccall).
[clangd] Fix use-after-free in XRefs
Tue, Feb 12, 2:39 AM
sammccall committed rL353821: [clangd] Fix use-after-free in XRefs.
[clangd] Fix use-after-free in XRefs
Tue, Feb 12, 2:38 AM
sammccall committed rCTE353821: [clangd] Fix use-after-free in XRefs.
[clangd] Fix use-after-free in XRefs
Tue, Feb 12, 2:38 AM

Mon, Feb 11

sammccall added a comment to D57888: [X86][SSE] Generalize X86ISD::BLENDI support to more value types (WIP).

I bootstrapped clang with this patch applied: on x86-64 with -O3 -msse4.2 and assertions off.

Mon, Feb 11, 10:42 AM · Restricted Project
sammccall added inline comments to D58062: Support framework import/include auto-completion.
Mon, Feb 11, 10:33 AM · Restricted Project
sammccall committed rGe825ba916568: Revert "[X86][SSE] Generalize X86ISD::BLENDI support to more value types" (authored by sammccall).
Revert "[X86][SSE] Generalize X86ISD::BLENDI support to more value types"
Mon, Feb 11, 6:06 AM
sammccall committed rL353699: Revert "[X86][SSE] Generalize X86ISD::BLENDI support to more value types".
Revert "[X86][SSE] Generalize X86ISD::BLENDI support to more value types"
Mon, Feb 11, 6:05 AM
sammccall accepted D58037: [clangd] Prefer location from codegen files when merging symbols..
Mon, Feb 11, 5:34 AM · Restricted Project
sammccall added a comment to D57943: [clangd] **Prototype**: clang-tidy-based tweaks.

This is an intriguing idea, and is at least useful to prototype new tweaks.

Mon, Feb 11, 2:25 AM · Restricted Project
sammccall added a comment to D57739: [clangd] Format tweak's replacements..

It's not about stability or whether the functionality is desired, but layering.
Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.

My argument is that it's better to provide formatting by default in the public interface for applying a single tweak.
I might be missing some use-cases, e.g. one that comes to mind is applying multiple tweaks in a row, in which case we'd want to format once and not for every tweak.

If providing formatting was free, I'd be fine with this, but it leaks into the public interface in two places: a) it requires the caller to plumb through a formatting style, and b) it is another source of errors.

Mon, Feb 11, 2:02 AM · Restricted Project
sammccall added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

As far as reworking the tests to use these functions, I've thought about that a bit:

  • These functions return AST nodes. It's not clear to me how I would come up with "expected" AST nodes to test the return values against.

See FindDecl

Mon, Feb 11, 1:32 AM · Restricted Project

Fri, Feb 8

sammccall accepted D57923: [Format/ObjC] Fix [foo bar]->baz formatting as lambda arrow.
Fri, Feb 8, 7:22 AM · Restricted Project
sammccall added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

Updated reviewers line to reflect (I think) current reality so this doesn't get lost, but feel free to reassign as you'd prefer.

Fri, Feb 8, 4:30 AM · Restricted Project
sammccall added reviewers for D56370: [clangd] Add support for type hierarchy (super types only for now): kadircet, sammccall.
Fri, Feb 8, 4:27 AM · Restricted Project
sammccall added a comment to D56370: [clangd] Add support for type hierarchy (super types only for now).

Implementation LG, but I am not sure about adding a functionality on a proposal that might change. WDYT @sammccall ?

Fri, Feb 8, 4:26 AM · Restricted Project
sammccall added a comment to D57739: [clangd] Format tweak's replacements..

I agree with this concern, and don't think this is an appropriate layer to be aware of styling or failing on format errors. Can we consider moving the formatting to clangdserver as originally planned?

clang-format seems to be somewhat stable, do we actually expect that to be a large problem in practice?
On the other side, I can't imagine any clients that don't need formatting? E.g. it's nice when tests give the same results as one would see in the final clangd and tests don't go through ClangdServer.

Fri, Feb 8, 3:39 AM · Restricted Project
sammccall added a comment to D57739: [clangd] Format tweak's replacements..

Could we move the code to the Tweak itself, so that all the callers would get the formatting? I.e.

class Tweak {
   Replacements apply() {  // This is now non-virtual.
     auto Replacements = doApply();
     return cleanupAndFormat(Replacements);
   }

protected:
   virtual Replacements doApply() = 0; // inheritors should implement this now. Note: the name is terrible, suggestions are welcome.
};

This would ensure the callers don't need to deal with the formatting on their own.

This seems introduce intrusive changes to the Tweak interface (Tweak will need to know the FormatStyle somehow), I think this might be done in another patch.

Fri, Feb 8, 2:22 AM · Restricted Project

Thu, Feb 7

sammccall accepted D57392: [clangd] Mention indexing in docs..

Sorry about delaying this.

Thu, Feb 7, 7:27 AM · Restricted Project
sammccall accepted D57878: [clangd] Use Dex for dynamic index by default..

(Just flipping the flag seems fine too)

Thu, Feb 7, 7:24 AM · Restricted Project
sammccall accepted D57879: [clangd] Fix an assertion failure in Selection..

Repro:

template <class T>
struct Foo {};

template <template<class> class /*cursor here*/U>
struct Foo<U<int>*> {};

I'm not sure how easy is that, but this should probably be fixed in the RecursiveASTVisitor<>. There's really no point in calling TraverseDecl(null)

Thu, Feb 7, 7:23 AM · Restricted Project, Restricted Project

Wed, Feb 6

sammccall updated the diff for D56610: [clangd] A code action to qualify an unqualified name.

Update matching to use Inputs.ASTSelection.
Cover some more cases of names (I think?)

Wed, Feb 6, 6:39 PM · Restricted Project
sammccall commandeered D56610: [clangd] A code action to qualify an unqualified name.

(Grabbing this as discussed offline)

Wed, Feb 6, 6:34 PM · Restricted Project
sammccall accepted D57021: [clangd] Suggest adding missing includes for typos (like include-fixer)..

Great stuff! Sorry for the delay here.

Wed, Feb 6, 11:55 AM · Restricted Project, Restricted Project
sammccall accepted D57815: [clangd] Add type boost to fuzzy find in Dex..
Wed, Feb 6, 5:59 AM · Restricted Project
sammccall added inline comments to D57021: [clangd] Suggest adding missing includes for typos (like include-fixer)..
Wed, Feb 6, 1:08 AM · Restricted Project, Restricted Project

Tue, Feb 5

sammccall accepted D57746: [clangd] Add CLI flag "-clang-tidy" to enable/disable running clang-tidy checks..
Tue, Feb 5, 11:28 PM · Restricted Project, Restricted Project

Fri, Feb 1

sammccall committed rCTE352953: [Clangd] textDocument/definition and textDocument/declaration "bounce" between….
[Clangd] textDocument/definition and textDocument/declaration "bounce" between…
Fri, Feb 1, 9:56 PM
sammccall committed rL352953: [Clangd] textDocument/definition and textDocument/declaration "bounce" between….
[Clangd] textDocument/definition and textDocument/declaration "bounce" between…
Fri, Feb 1, 9:55 PM
sammccall closed D57580: [Clangd] textDocument/definition and textDocument/declaration "bounce" between definition and declaration location when they are distinct..
Fri, Feb 1, 9:55 PM · Restricted Project, Restricted Project
sammccall added a comment to D57570: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches..

Maybe we should consider stopping the traversals up at some points, e.g. at lambda decls or something similar to limit this class of problems in the future?

If you mean in some frameworky sort of way... yeah, it's quite likely we need something. At least to make the Node* -> DynTypedNode -> Stmt -> IfStmt dance less clunky.
I've deliberately avoided adding too much in the way of helpers for now in favor of the raw data structures, mostly wanting to see what the common patterns are - we can afford to write a few of these by hand.

Exactly, the code dealing with typed nodes there is definitely too verbose. Thinking about helpers like findAncestor<IfStmt>().
And yeah, totally fine to keep it as is for now, it's great that the change is focused.

e.g. findAncestor is nice but here we want to walk up but break if we hit a limit node (e.g. compoundstmt).
If that turns out to be the normal case, then a plain findAncestor isn't such a useful primitive.
Some other thoughts:

  • In a draft I did have a tree traversal where the node visitor could return {continue, abort, prune} - seemed cool but I didn't actually have a use for it
  • just some direct casting operators (Node->as<IfStmt>()) would help, probably.
Fri, Feb 1, 7:10 AM · Restricted Project, Restricted Project
sammccall committed rCTE352876: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches..
[clangd] Expose SelectionTree to code tweaks, and use it for swap if branches.
Fri, Feb 1, 7:09 AM
sammccall committed rL352876: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches..
[clangd] Expose SelectionTree to code tweaks, and use it for swap if branches.
Fri, Feb 1, 7:09 AM
sammccall closed D57570: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches..
Fri, Feb 1, 7:09 AM · Restricted Project, Restricted Project
sammccall committed rL352875: [clangd] Lib to compute and represent selection under cursor..
[clangd] Lib to compute and represent selection under cursor.
Fri, Feb 1, 7:09 AM
sammccall committed rCTE352875: [clangd] Lib to compute and represent selection under cursor..
[clangd] Lib to compute and represent selection under cursor.
Fri, Feb 1, 7:09 AM
sammccall committed rCTE352874: [clangd] Lib to compute and represent selection under cursor..
[clangd] Lib to compute and represent selection under cursor.
Fri, Feb 1, 7:05 AM
sammccall committed rL352874: [clangd] Lib to compute and represent selection under cursor..
[clangd] Lib to compute and represent selection under cursor.
Fri, Feb 1, 7:05 AM
sammccall closed D57562: [clangd] Lib to compute and represent selection under cursor..
Fri, Feb 1, 7:05 AM · Restricted Project, Restricted Project
sammccall updated the diff for D57570: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches..

Split declarations, add test for two-if-statements.

Fri, Feb 1, 5:56 AM · Restricted Project, Restricted Project
sammccall updated the diff for D57562: [clangd] Lib to compute and represent selection under cursor..

Review comments

Fri, Feb 1, 5:42 AM · Restricted Project, Restricted Project
sammccall added a comment to D57562: [clangd] Lib to compute and represent selection under cursor..

The "select everything in a very large file" is exactly the problematic case that came to mind before. I'm sure it's possible to build the selection tree in linear time, but the traversals might also become a problem.

(Just so we're on the same page: this is good discussion regardless, but do you see this as an issue we should try and resolve in this patch?)

Fri, Feb 1, 5:38 AM · Restricted Project, Restricted Project
sammccall added a comment to D56653: [clangd] Penalize file-scope symbols in the ranking for non-completion queries.

Friendly reminder than this is ready to be committed :)

Fri, Feb 1, 5:09 AM · Restricted Project
sammccall committed rL352868: [clangd] Penalize file-scope symbols in the ranking for non-completion queries.
[clangd] Penalize file-scope symbols in the ranking for non-completion queries
Fri, Feb 1, 5:07 AM
sammccall committed rCTE352868: [clangd] Penalize file-scope symbols in the ranking for non-completion queries.
[clangd] Penalize file-scope symbols in the ranking for non-completion queries
Fri, Feb 1, 5:07 AM
sammccall closed D56653: [clangd] Penalize file-scope symbols in the ranking for non-completion queries.
Fri, Feb 1, 5:07 AM · Restricted Project
sammccall created D57580: [Clangd] textDocument/definition and textDocument/declaration "bounce" between definition and declaration location when they are distinct..
Fri, Feb 1, 4:50 AM · Restricted Project, Restricted Project
sammccall committed rL352864: [clangd] Implement textDocument/declaration from LSP 3.14.
[clangd] Implement textDocument/declaration from LSP 3.14
Fri, Feb 1, 3:26 AM
sammccall committed rCTE352864: [clangd] Implement textDocument/declaration from LSP 3.14.
[clangd] Implement textDocument/declaration from LSP 3.14
Fri, Feb 1, 3:26 AM
sammccall closed D57388: [clangd] Implement textDocument/declaration from LSP 3.14.
Fri, Feb 1, 3:26 AM · Restricted Project
sammccall committed rCTE352863: [clangd] Use delimited style to make life easier for the fuzzer.
[clangd] Use delimited style to make life easier for the fuzzer
Fri, Feb 1, 3:20 AM
sammccall committed rL352863: [clangd] Use delimited style to make life easier for the fuzzer.
[clangd] Use delimited style to make life easier for the fuzzer
Fri, Feb 1, 3:20 AM
sammccall committed rL352857: [clangd] Unbreak fuzzer target.
[clangd] Unbreak fuzzer target
Fri, Feb 1, 3:09 AM
sammccall committed rCTE352857: [clangd] Unbreak fuzzer target.
[clangd] Unbreak fuzzer target
Fri, Feb 1, 3:09 AM
sammccall updated the diff for D57388: [clangd] Implement textDocument/declaration from LSP 3.14.

address review comments

Fri, Feb 1, 2:51 AM · Restricted Project
sammccall added inline comments to D57388: [clangd] Implement textDocument/declaration from LSP 3.14.
Fri, Feb 1, 2:50 AM · Restricted Project
sammccall updated the diff for D57570: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches..

Simplify Selection constructor.
Don't traverse up through compoundstmt.
Add more tests.

Fri, Feb 1, 2:41 AM · Restricted Project, Restricted Project
sammccall added a comment to D57570: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches..

I wonder if this makes the check available in too many places?

void test() {
  if (callFunc([]() { 
         for (int i = 0; i < 100; ++i) { /* compute something */; } // action also available here...
      }) {
   } else {} 
}

This look a bit artificial for this check as conditions are rarely large, but could be more significant for other checks, where it could be more common to have declarations of classes, functions and lambdas lambdas recursively inside other declarations.
Maybe we should consider stopping the traversals up at some points, e.g. at lambda decls or something similar to limit this class of problems in the future?

Heh, this actually makes it easier to implement - we can break at a compoundstmt, and since we require both if and then clauses to be braced, this takes care of only alllowing subexpressions of cond.

Fri, Feb 1, 2:40 AM · Restricted Project, Restricted Project