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 (178 w, 10 h)
Roles
Administrator

Recent Activity

Today

sammccall created D73369: [clangd] Simplify "preferred" vs "definition" logic a bit in XRefs AST code..
Fri, Jan 24, 9:35 AM · Restricted Project
sammccall created D73367: [clangd] Go-to-definition on 'override' jumps to overridden method(s).
Fri, Jan 24, 9:26 AM · Restricted Project
sammccall added a comment to D73354: clang-format: insert trailing commas into containers..

I have an issue with this change. Currently (at least for C++), the presence of a trailing comma is used as a formatting hint to put all the element in one line or one per line as below:

Good point about the interaction with bin-packing.
Do the style guides that want this actually endorse bin-packing containers? Insertion + bin-packing + comma-as-hint basically can't coexist - we could turn off the hint or make this option mutually exclusive with bin-packing.

Fri, Jan 24, 8:40 AM · Restricted Project
sammccall added a comment to D73354: clang-format: insert trailing commas into containers..

For rolling this out, I think the best path is:

  • check in the option, but don't turn it on with any styles yet
  • test it by taking a large codebase, formatting it (normal options), format it (with comma insertion), look at the diffs (internally google3diff can do this)
  • turn it on for -style=google
Fri, Jan 24, 8:14 AM · Restricted Project
sammccall committed rG76fa5d50f3d1: [clangd] Remove pesky ;. NFC (authored by sammccall).
[clangd] Remove pesky ;. NFC
Fri, Jan 24, 7:17 AM
sammccall committed rG6ef1ccecf7ae: [clangd] Rename DiagnosticsConsumer -> ClangdServer::Callbacks, and make it… (authored by sammccall).
[clangd] Rename DiagnosticsConsumer -> ClangdServer::Callbacks, and make it…
Fri, Jan 24, 7:00 AM
sammccall closed D73346: [clangd] Rename DiagnosticsConsumer -> ClangdServer::Callbacks, and make it optional.
Fri, Jan 24, 7:00 AM · Restricted Project
sammccall added inline comments to D73346: [clangd] Rename DiagnosticsConsumer -> ClangdServer::Callbacks, and make it optional.
Fri, Jan 24, 6:59 AM · Restricted Project
sammccall accepted D73344: [clangd][Hover] Handle uninstantiated templates.

Can you file a new bug or modify the upstream one to audit existing uses of getTemplateInstantiationPattern() (and maybe related functions)?

Fri, Jan 24, 5:28 AM · Restricted Project
sammccall updated the diff for D73346: [clangd] Rename DiagnosticsConsumer -> ClangdServer::Callbacks, and make it optional.

Document new interface and threading model of existing functions.

Fri, Jan 24, 5:19 AM · Restricted Project
sammccall created D73346: [clangd] Rename DiagnosticsConsumer -> ClangdServer::Callbacks, and make it optional.
Fri, Jan 24, 5:11 AM · Restricted Project
sammccall committed rG7d20e80225b3: [clangd] Show background index status using LSP 3.15 work-done progress… (authored by sammccall).
[clangd] Show background index status using LSP 3.15 work-done progress…
Fri, Jan 24, 3:22 AM
sammccall closed D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications.
Fri, Jan 24, 3:22 AM · Restricted Project, Restricted Project
sammccall accepted D73336: [clangd][Hover] Change arrow in return type back to →.
Fri, Jan 24, 3:13 AM · Restricted Project
sammccall committed rGd3260bf5b2f7: [clangd] Errors in TestTU cause test failures unless suppressed with error-ok. (authored by sammccall).
[clangd] Errors in TestTU cause test failures unless suppressed with error-ok.
Fri, Jan 24, 2:17 AM
sammccall closed D73199: [clangd] Errors in TestTU cause test failures unless suppressed with error-ok..
Fri, Jan 24, 2:17 AM · Restricted Project
sammccall added inline comments to D73199: [clangd] Errors in TestTU cause test failures unless suppressed with error-ok..
Fri, Jan 24, 2:08 AM · Restricted Project
sammccall committed rGb3b68c0f802e: [Format] Fix 'auto x(T&&, T &&)->F' with PAS_Left. (authored by sammccall).
[Format] Fix 'auto x(T&&, T &&)->F' with PAS_Left.
Fri, Jan 24, 2:05 AM
sammccall closed D73334: [Format] Fix 'auto x(T&&, T &&)->F' with PAS_Left..
Fri, Jan 24, 2:05 AM · Restricted Project
sammccall updated the diff for D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications.

address comments

Fri, Jan 24, 1:32 AM · Restricted Project, Restricted Project
sammccall added inline comments to D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications.
Fri, Jan 24, 1:32 AM · Restricted Project, Restricted Project
sammccall created D73334: [Format] Fix 'auto x(T&&, T &&)->F' with PAS_Left..
Fri, Jan 24, 1:15 AM · Restricted Project
sammccall added inline comments to D73334: [Format] Fix 'auto x(T&&, T &&)->F' with PAS_Left..
Fri, Jan 24, 1:15 AM · Restricted Project

Yesterday

sammccall created D73278: [clangd] Type: ⇨ ∈; 🡺 ⇨ →.
Thu, Jan 23, 10:09 AM · Restricted Project
sammccall added a comment to D73271: [clang][CodeComplete] Support for designated initializers.

oops, needs some more tests.

Thu, Jan 23, 9:29 AM · Restricted Project
sammccall accepted D73271: [clang][CodeComplete] Support for designated initializers.

Nit: maybe mention "top-level" in the patch?

Thu, Jan 23, 7:07 AM · Restricted Project
sammccall committed rG5c02fe1faabd: Revert "[Concepts] Placeholder constraints and abbreviated templates" (authored by sammccall).
Revert "[Concepts] Placeholder constraints and abbreviated templates"
Thu, Jan 23, 2:00 AM
sammccall added a reverting change for rGe57a9abc4b01: [Concepts] Placeholder constraints and abbreviated templates: rG5c02fe1faabd: Revert "[Concepts] Placeholder constraints and abbreviated templates".
Thu, Jan 23, 2:00 AM

Wed, Jan 22

sammccall updated the diff for D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications.

revert accidental change

Wed, Jan 22, 10:51 AM · Restricted Project, Restricted Project
sammccall updated the diff for D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications.

Revert changes to VSCode client. This experimental version of the VSCode libs
is fairly new and some corp mirrors we care about are behind ;-)

Wed, Jan 22, 10:51 AM · Restricted Project, Restricted Project
sammccall created D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications.
Wed, Jan 22, 10:51 AM · Restricted Project, Restricted Project
sammccall created D73199: [clangd] Errors in TestTU cause test failures unless suppressed with error-ok..
Wed, Jan 22, 7:43 AM · Restricted Project
sammccall accepted D73177: [clang][CodeComplete] Make completion work after initializer lists.
Wed, Jan 22, 6:43 AM · Restricted Project
sammccall added a comment to D72647: [clangd] Only re-open files if their flags changed.

Let's make the minimal change here and land this.

Wed, Jan 22, 2:12 AM · Restricted Project
sammccall added inline comments to D72867: [clangd] Support renaming designated initializers.
Wed, Jan 22, 2:08 AM · Restricted Project
sammccall accepted D73124: [clangd] Add C++20 concepts support to findExplicitReferences() and semantic highlighting.
Wed, Jan 22, 1:59 AM · Restricted Project
sammccall accepted D73140: [clangd] Add C++20 concepts support to TargetFinder.
Wed, Jan 22, 1:54 AM · Restricted Project

Tue, Jan 21

sammccall added a comment to D72874: [clangd] Add a textual fallback for go-to-definition.

I've tried this out locally and it's fun! As suspected on the bug though, IMO it's far from accurate enough. Examples from clangd/Compiler.cpp:

  • it triggers on almost every word, even words that plainly don't refer to any decl like format [[lazily]], in case vlog is off. This means that e.g. (in VSCode) the underline on ctrl-hover gives no/misleading signal. It also means that missing your target now jumps you somewhere random instead of doing nothing.
  • when it works properly, the correct result usually mixed with incorrect results (e.g. createInvocationFromCommandLine sets [[DisableFree]]).
  • it doesn't work for some symbols - ones that are not indexable (e.g. RemappedFileBuffers will handle the lifetime of the [[Buffer]] pointer, gives a variety of wrong results)
Tue, Jan 21, 11:17 AM · Restricted Project
sammccall accepted D73102: [clangd] Handle the missing injectedClassNameType in targetDecl..
Tue, Jan 21, 8:07 AM · Restricted Project
sammccall accepted D73110: [clangd] Drop returntype/type when hovering over type-ish names.
Tue, Jan 21, 8:05 AM · Restricted Project
sammccall accepted D72581: [Syntax] Add mapping from spelled to expanded tokens for TokenBuffer.
Tue, Jan 21, 3:49 AM · Restricted Project
sammccall added a comment to D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times.

Thanks for fixing this. I think it could maybe use a further tweak, LMK if I'm missing something as my intuition around macros is pretty bad.

Tue, Jan 21, 3:15 AM · Restricted Project
sammccall added inline comments to D72867: [clangd] Support renaming designated initializers.
Tue, Jan 21, 2:38 AM · Restricted Project

Fri, Jan 17

sammccall committed rGd035c832c3f9: [lldb] Try to fix writing outside temp dir from… (authored by sammccall).
[lldb] Try to fix writing outside temp dir from…
Fri, Jan 17, 8:37 AM

Thu, Jan 16

sammccall added a comment to D72883: [clangd][test] Disable a particular testcase in FindExplicitReferencesTest when LLVM_ENABLE_EXPENSIVE_CHECKS.

Please revert this, it was already fixed by d54d71b67e60

Thu, Jan 16, 3:24 PM · Restricted Project
sammccall added a comment to D72883: [clangd][test] Disable a particular testcase in FindExplicitReferencesTest when LLVM_ENABLE_EXPENSIVE_CHECKS.

kadir has a pending (or landed?) fix for this.

Thu, Jan 16, 3:15 PM · Restricted Project
sammccall added a comment to D72326: [clang-format] Rebased on master: Add option to specify explicit config file.

In this patch, relative paths are relative to the working directory (or at least the current directory of the VFS).

Thu, Jan 16, 7:30 AM · Restricted Project, Restricted Project
sammccall accepted D72498: [clangd] Print underlying type for decltypes in hover.

Still LG

Thu, Jan 16, 7:11 AM · Restricted Project
sammccall accepted D72840: [clangd] Make define outline code action visible.
Thu, Jan 16, 7:03 AM · Restricted Project
sammccall accepted D72826: [clangd] Make output order of allTargetDecls deterministic.
Thu, Jan 16, 4:30 AM · Restricted Project
sammccall accepted D72826: [clangd] Make output order of allTargetDecls deterministic.
Thu, Jan 16, 3:23 AM · Restricted Project
sammccall added a comment to D72769: Replace CLANG_SPAWN_CC1 env var with a driver mode flag.

Thanks for fixing this.

Thu, Jan 16, 1:03 AM

Tue, Jan 14

sammccall accepted D72500: [clangd] Show hover info for expressions.

LG!

Tue, Jan 14, 10:14 AM · Restricted Project
sammccall added inline comments to D72622: [clangd] Add a ruler after header in hover.
Tue, Jan 14, 10:14 AM · Restricted Project
sammccall added inline comments to D72500: [clangd] Show hover info for expressions.
Tue, Jan 14, 3:02 AM · Restricted Project
sammccall committed rG41b520188820: [Target] Fix uninitialized value in 10c11e4e2d05cf0e8f8251f50d84ce77eb1e9b8d (authored by sammccall).
[Target] Fix uninitialized value in 10c11e4e2d05cf0e8f8251f50d84ce77eb1e9b8d
Tue, Jan 14, 2:33 AM
sammccall accepted D72623: [clangd] Rearrange type, returntype and parameters in hover card.
Tue, Jan 14, 2:28 AM · Restricted Project
sammccall committed rG547abdd921e4: [mlir] Fix -Wunused (authored by sammccall).
[mlir] Fix -Wunused
Tue, Jan 14, 1:13 AM
sammccall added inline comments to D72647: [clangd] Only re-open files if their flags changed.
Tue, Jan 14, 12:17 AM · Restricted Project

Mon, Jan 13

sammccall created D72634: [clangd] Improve ObjC property handling in SelectionTree..
Mon, Jan 13, 10:54 AM · Restricted Project
sammccall accepted D72622: [clangd] Add a ruler after header in hover.
Mon, Jan 13, 10:35 AM · Restricted Project
sammccall accepted D72623: [clangd] Rearrange type, returntype and parameters in hover card.
Mon, Jan 13, 10:25 AM · Restricted Project
sammccall accepted D72625: [clangd] Render header of hover card as a heading.
Mon, Jan 13, 10:17 AM · Restricted Project
sammccall accepted D72594: [clangd] Include expression in DecltypeTypeLoc sourcerange while building SelectionTree.

Thanks!

Mon, Jan 13, 10:16 AM · Restricted Project
sammccall added a comment to rGa5994c789a29: [X86][Disassembler] Simplify and optimize reader functions.

This appears to have broken bounds checking in some cases.
There don't seem to be any in-tree tests, but the exegesis DisassemblerTest.TooShortABuffer hits an LLVM assertion:

Mon, Jan 13, 8:41 AM
sammccall committed rG0b91e78a7190: Add missing triples to tests in 0c29d3ff2233696f663ae34a8aeda23c750ac68f so… (authored by sammccall).
Add missing triples to tests in 0c29d3ff2233696f663ae34a8aeda23c750ac68f so…
Mon, Jan 13, 7:06 AM
sammccall added a comment to D72498: [clangd] Print underlying type for decltypes in hover.

Currently, I think that in most cases, showing both expanded (canonical) and spelled types is sufficient.

This has been used in ycmd for ~4 years without complaint. https://github.com/clangd/clangd/issues/58#issuecomment-507800970

That actually doesn't look bad. Maybe let's try doing that and see whether we'll get negative feedback?
That seems to give useful information in all cases, so at least it'll cover all use-cases even it's more verbose.

What do others think?

SGTM, happy to update all types in HoverInfo to contain both a pretty-printed and canonical version. Where pretty-printed would just means desugared, so keywords like auto/decltype would've
been stripped away and it would be the type as written in all other cases, while canonical would refer to clang canonical with all of the type aliases etc. resolved. Ofc.
Does that SG to you as well @sammccall ?

Mon, Jan 13, 6:54 AM · Restricted Project
sammccall committed rGe45fcfc3aa57: Revert "[DWARF5][clang]: Added support for DebugInfo generation for auto return… (authored by sammccall).
Revert "[DWARF5][clang]: Added support for DebugInfo generation for auto return…
Mon, Jan 13, 2:16 AM
sammccall added a reverting change for rG6d6a4590c5d4: [DWARF5][clang]: Added support for DebugInfo generation for auto return type…: rGe45fcfc3aa57: Revert "[DWARF5][clang]: Added support for DebugInfo generation for auto return….
Mon, Jan 13, 2:16 AM
sammccall added a comment to D70524: Support DebugInfo generation for auto return type for C++ functions..

Following crashes for me with clang++ -g2:

Mon, Jan 13, 2:16 AM · debug-info, Restricted Project, Restricted Project
sammccall added a comment to D70524: Support DebugInfo generation for auto return type for C++ functions..

Following crashes for me with clang++ -g2:

Mon, Jan 13, 2:02 AM · debug-info, Restricted Project, Restricted Project
sammccall added a comment to D70524: Support DebugInfo generation for auto return type for C++ functions..

This seems to have introduced a crash compiling libcxx. I'm currently reducing the crashing code.

Mon, Jan 13, 1:43 AM · debug-info, Restricted Project, Restricted Project
sammccall added a comment to D72498: [clangd] Print underlying type for decltypes in hover.
  • hover over the front , you'll see "instance-method frontstd::vector<int, class std::allocator<int> >::reference".
  • hover over the push_back, you'll see "std::vector<int, class std::allocator<int> >::value_type && __x".

These look terrible and are the great examples where showing canonical types results in better output than canonical types.
I wonder why we add std::vector<int, class std::allocator<int>>:: in the first place, I believe the standard library uses value_type in the declaration. Showing value_type is not great, but at least that doesn't uglify what was written in the code in the first place.
FWIW, I think the perfect output in those cases would be int (aka value_type)

Mon, Jan 13, 12:06 AM · Restricted Project

Fri, Jan 10

sammccall committed rG4c5a4514d145: [clangd] Fix targetDecl() on certain usage of ObjC properties. (authored by sammccall).
[clangd] Fix targetDecl() on certain usage of ObjC properties.
Fri, Jan 10, 9:08 AM
sammccall closed D72494: [clangd] Fix targetDecl() on certain usage of ObjC properties..
Fri, Jan 10, 9:08 AM · Restricted Project
sammccall added a comment to D72498: [clangd] Print underlying type for decltypes in hover.

I think i'm also comfortable with marking the linked bug as wontfix.

The previous example is just minimal repo.
I think it's worth fixing in this case.

template<typename T1, typename T2>  
auto sum(T1 &t1, T2 &t2) ->decltype(t1 + t2))
{  
	return t1 + t2;  
}
Fri, Jan 10, 9:07 AM · Restricted Project
sammccall removed a child revision for D72494: [clangd] Fix targetDecl() on certain usage of ObjC properties.: D72508: [clangd] Support pseudo-obj expr, opaque values, and property references in findExplicitReferences().
Fri, Jan 10, 7:33 AM · Restricted Project
sammccall added inline comments to D72508: [clangd] Support pseudo-obj expr, opaque values, and property references in findExplicitReferences().
Fri, Jan 10, 7:33 AM · Restricted Project
sammccall removed a parent revision for D72508: [clangd] Support pseudo-obj expr, opaque values, and property references in findExplicitReferences(): D72494: [clangd] Fix targetDecl() on certain usage of ObjC properties..
Fri, Jan 10, 7:33 AM · Restricted Project
sammccall added a parent revision for D72508: [clangd] Support pseudo-obj expr, opaque values, and property references in findExplicitReferences(): D72494: [clangd] Fix targetDecl() on certain usage of ObjC properties..
Fri, Jan 10, 7:33 AM · Restricted Project
sammccall added a child revision for D72494: [clangd] Fix targetDecl() on certain usage of ObjC properties.: D72508: [clangd] Support pseudo-obj expr, opaque values, and property references in findExplicitReferences().
Fri, Jan 10, 7:33 AM · Restricted Project
sammccall created D72508: [clangd] Support pseudo-obj expr, opaque values, and property references in findExplicitReferences().
Fri, Jan 10, 7:33 AM · Restricted Project
sammccall added a comment to D72498: [clangd] Print underlying type for decltypes in hover.

I think i'm also comfortable with marking the linked bug as wontfix.

Fri, Jan 10, 6:36 AM · Restricted Project
sammccall added a comment to D72500: [clangd] Show hover info for expressions.

Basing this on the hover for the type doesn't seem right. e.g. int should be the Type rather than the Name.

Fri, Jan 10, 5:47 AM · Restricted Project
sammccall added a comment to D72498: [clangd] Print underlying type for decltypes in hover.

Yeah we discussed that case offline and I should have mentioned.
Resolving this in places other than the top-level would be nice and probably worthy of a comment at least. But this special case seems common enough to be worth having.
The only place fully resolving could reasonably be done I guess is TypePrinter, because we can't actually transform the types into the correct form IIUC.
It's particularly unclear to me why typeprinter descends into auto but prints decltype, but Kadir says that seems to be intentional.

Fri, Jan 10, 5:47 AM · Restricted Project
sammccall added a comment to D72494: [clangd] Fix targetDecl() on certain usage of ObjC properties..

Could you also add a test (and possibly support this in the visitors) for findExplicitReferences?

I've written tests, but it doesn't work and it's not obvious to me yet how to make it work, so I'll do this in a separate patch.

By which I mean, adding the same delegation to the RAV there doesn't work, and I'm not confident that jiggling it around until the test passes is correct here, so I want to understand the traversal behavior a bit better first (RAV has several special cases around OVE/POE)

Fri, Jan 10, 4:21 AM · Restricted Project
sammccall added a comment to D72494: [clangd] Fix targetDecl() on certain usage of ObjC properties..

Could you also add a test (and possibly support this in the visitors) for findExplicitReferences?

Fri, Jan 10, 4:10 AM · Restricted Project
sammccall created D72499: [clangd] (DO NOT COMMIT) hovering on "md" raw strings displays those..
Fri, Jan 10, 3:46 AM · Restricted Project
sammccall accepted D72498: [clangd] Print underlying type for decltypes in hover.
Fri, Jan 10, 3:46 AM · Restricted Project
sammccall created D72494: [clangd] Fix targetDecl() on certain usage of ObjC properties..
Fri, Jan 10, 1:47 AM · Restricted Project
sammccall accepted D72450: [clangd] Improve type printing in hover.

Nice!

Fri, Jan 10, 12:24 AM · Restricted Project
sammccall accepted D72462: [clangd] Fix markdown rendering in VSCode.
Fri, Jan 10, 12:12 AM · Restricted Project

Thu, Jan 9

sammccall accepted D71555: [clangd] Refurbish HoverInfo::present.

still LG, go ahead and we can iterate

Thu, Jan 9, 1:32 AM · Restricted Project

Wed, Jan 8

sammccall accepted D72415: [clangd] Respect `--sysroot` argument if it is set.

I wonder what accident of history led to the different number of dashes between the flags, weird.

Wed, Jan 8, 2:32 PM · Restricted Project
sammccall accepted D71555: [clangd] Refurbish HoverInfo::present.

Great stuff, let's finally ship it!

Wed, Jan 8, 9:33 AM · Restricted Project
sammccall added inline comments to D71555: [clangd] Refurbish HoverInfo::present.
Wed, Jan 8, 3:51 AM · Restricted Project

Tue, Jan 7

sammccall accepted D71422: [clangd] Introduce bulletlists.
Tue, Jan 7, 4:18 AM · Restricted Project
sammccall committed rGc69ae835d0e0: [clangd] Add path mappings functionality (authored by sammccall).
[clangd] Add path mappings functionality
Tue, Jan 7, 3:41 AM
sammccall closed D64305: [clangd] Add path mappings functionality.
Tue, Jan 7, 3:41 AM · Restricted Project
sammccall added a comment to D64305: [clangd] Add path mappings functionality.

Thanks! The latest snapshot looks good. Landing it now with a few minor tweaks mentioned below.

Tue, Jan 7, 3:04 AM · Restricted Project