Page MenuHomePhabricator

sammccall (Sam McCall)
UserAdministrator

Projects

User does not belong to any projects.

User Details

User Since
Aug 26 2016, 6:53 AM (230 w, 8 h)
Roles
Administrator

Recent Activity

Today

sammccall retitled D95057: [clangd] Allow configuration database to be specified in config. from [clangd] WIP: configurable compilation database directory to [clangd] Allow configuration database to be specified in config..
Fri, Jan 22, 7:38 AM · Restricted Project
sammccall updated the diff for D95057: [clangd] Allow configuration database to be specified in config..

Add tests and fix bugs

Fri, Jan 22, 7:37 AM · Restricted Project
sammccall updated the diff for D95057: [clangd] Allow configuration database to be specified in config..

Rebase, implement yaml bits (no tests yet)

Fri, Jan 22, 5:44 AM · Restricted Project
sammccall committed rG60cd75a098d4: [clangd] Inject context provider rather than config into ClangdServer. NFC (authored by sammccall).
[clangd] Inject context provider rather than config into ClangdServer. NFC
Fri, Jan 22, 5:34 AM
sammccall closed D95087: [clangd] Inject context provider rather than config into ClangdServer. NFC.
Fri, Jan 22, 5:34 AM · Restricted Project
sammccall accepted D95206: [clangd][SwapIndex] ensure that the old index is alive while we are using it via the function returned by `SwapIndex::indexedFiles()` call.
Fri, Jan 22, 5:17 AM · Restricted Project
sammccall accepted D95206: [clangd][SwapIndex] ensure that the old index is alive while we are using it via the function returned by `SwapIndex::indexedFiles()` call.

Thanks, I'd seen these crashes but not manage to track down yet!

Fri, Jan 22, 1:30 AM · Restricted Project

Wed, Jan 20

sammccall requested review of D95087: [clangd] Inject context provider rather than config into ClangdServer. NFC.
Wed, Jan 20, 1:39 PM · Restricted Project
sammccall requested review of D95057: [clangd] Allow configuration database to be specified in config..
Wed, Jan 20, 9:35 AM · Restricted Project
sammccall requested review of D95031: [clangd] Log warning when using legacy (theia) semantic highlighting..
Wed, Jan 20, 3:39 AM · Restricted Project
sammccall committed rG2ab5fd2c8567: [clangd] Retire some flags for uncontroversial, stable features. (authored by sammccall).
[clangd] Retire some flags for uncontroversial, stable features.
Wed, Jan 20, 2:49 AM
sammccall closed D94727: [clangd] Retire some flags for uncontroversial, stable features..
Wed, Jan 20, 2:49 AM · Restricted Project
sammccall committed rGe6be5c7cd6d2: [clangd] Remove the recovery-ast options. (authored by sammccall).
[clangd] Remove the recovery-ast options.
Wed, Jan 20, 2:31 AM
sammccall closed D94724: [clangd] Remove the recovery-ast options..
Wed, Jan 20, 2:31 AM · Restricted Project
sammccall added inline comments to D94727: [clangd] Retire some flags for uncontroversial, stable features..
Wed, Jan 20, 2:31 AM · Restricted Project
sammccall committed rGde4ba7073bd7: [clangd] Move DirBasedCDB broadcasting onto its own thread. (authored by sammccall).
[clangd] Move DirBasedCDB broadcasting onto its own thread.
Wed, Jan 20, 2:23 AM
sammccall closed D94606: [clangd] Move DirBasedCDB broadcasting onto its own thread..
Wed, Jan 20, 2:23 AM · Restricted Project
sammccall committed rG536a1b0ea211: [clangd] Allow CDBs to have background work to block on. (authored by sammccall).
[clangd] Allow CDBs to have background work to block on.
Wed, Jan 20, 2:11 AM
sammccall closed D94603: [clangd] Allow CDBs to have background work to block on..
Wed, Jan 20, 2:11 AM · Restricted Project
sammccall updated subscribers of D94769: [Support] Format provider improvements.
Wed, Jan 20, 2:08 AM · Restricted Project

Mon, Jan 18

sammccall added inline comments to D92290: [clangd] Factor out the heuristic resolver code into its own class.
Mon, Jan 18, 12:17 PM · Restricted Project
sammccall added a comment to D92290: [clangd] Factor out the heuristic resolver code into its own class.

Thanks a lot for doing this!
I have some thoughts on how we can give it a nice API/more clearly separate it from FindTarget/improve internal structure.

Mon, Jan 18, 9:36 AM · Restricted Project
sammccall added inline comments to D94606: [clangd] Move DirBasedCDB broadcasting onto its own thread..
Mon, Jan 18, 8:07 AM · Restricted Project
sammccall updated the diff for D94606: [clangd] Move DirBasedCDB broadcasting onto its own thread..

address comments

Mon, Jan 18, 8:07 AM · Restricted Project
sammccall added a comment to D94769: [Support] Format provider improvements.

Can you add a test for whatever this is fixing?

Mon, Jan 18, 7:49 AM · Restricted Project
sammccall accepted D94755: [clangd] Fix division by zero when computing scores.
Mon, Jan 18, 5:56 AM · Restricted Project

Thu, Jan 14

sammccall committed rG4183999e0fe1: [clangd] Reduce logspam for CDB scanning (authored by sammccall).
[clangd] Reduce logspam for CDB scanning
Thu, Jan 14, 2:55 PM
sammccall requested review of D94727: [clangd] Retire some flags for uncontroversial, stable features..
Thu, Jan 14, 2:51 PM · Restricted Project
sammccall requested review of D94724: [clangd] Remove the recovery-ast options..
Thu, Jan 14, 2:27 PM · Restricted Project
sammccall accepted D94699: [clangd] Set correct CWD when using compile_flags.txt.

Yikes, thanks!

Thu, Jan 14, 2:10 PM · Restricted Project
sammccall accepted D94424: [clangd] Make AST-based signals available to runWithPreamble..

Thanks!

Thu, Jan 14, 8:26 AM · Restricted Project
sammccall committed rG17fb21f875f4: [clangd] Remove another option that was effectively always true. NFC (authored by sammccall).
[clangd] Remove another option that was effectively always true. NFC
Thu, Jan 14, 8:20 AM

Wed, Jan 13

sammccall added a comment to D94424: [clangd] Make AST-based signals available to runWithPreamble..

Thanks, this looks pretty solid!

Wed, Jan 13, 12:55 PM · Restricted Project
sammccall committed rG0bbc6a6bb643: [clangd] Remove some old CodeCompletion options that are never (un)set. NFC (authored by sammccall).
[clangd] Remove some old CodeCompletion options that are never (un)set. NFC
Wed, Jan 13, 9:02 AM
sammccall committed rG466acd694861: [clangd] Avoid reallocating buffers for each message read: (authored by sammccall).
[clangd] Avoid reallocating buffers for each message read:
Wed, Jan 13, 8:41 AM
sammccall closed D93653: [clangd] Avoid reallocating buffers for each message read:.
Wed, Jan 13, 8:40 AM · Restricted Project
sammccall committed rG66d5994bd38a: [clangd] Explicitly avoid background-indexing the same file twice. (authored by sammccall).
[clangd] Explicitly avoid background-indexing the same file twice.
Wed, Jan 13, 8:29 AM
sammccall closed D94503: [clangd] Explicitly avoid background-indexing the same file twice..
Wed, Jan 13, 8:29 AM · Restricted Project
sammccall accepted D94513: [clangd] Remove "decision-forest-base" experimental flag..
Wed, Jan 13, 8:19 AM · Restricted Project
sammccall requested review of D94606: [clangd] Move DirBasedCDB broadcasting onto its own thread..
Wed, Jan 13, 8:13 AM · Restricted Project
sammccall requested review of D94603: [clangd] Allow CDBs to have background work to block on..
Wed, Jan 13, 7:50 AM · Restricted Project
sammccall committed rG90164ba957a2: [clangd] Split out a base class for delegating GlobalCompilationDatabases. NFC (authored by sammccall).
[clangd] Split out a base class for delegating GlobalCompilationDatabases. NFC
Wed, Jan 13, 7:20 AM

Tue, Jan 12

sammccall retitled D94503: [clangd] Explicitly avoid background-indexing the same file twice. from [clangd] Avoid having the same file in the queue twice in the background index. to [clangd] Explicitly avoid background-indexing the same file twice..
Tue, Jan 12, 10:13 AM · Restricted Project
sammccall updated the diff for D94503: [clangd] Explicitly avoid background-indexing the same file twice..

In fact, never index the same file twice at all.

Tue, Jan 12, 10:10 AM · Restricted Project
sammccall requested review of D94503: [clangd] Explicitly avoid background-indexing the same file twice..
Tue, Jan 12, 8:48 AM · Restricted Project
sammccall added a comment to D93829: [clangd] Support outgoing calls in call hierarchy.

Thanks for looking into this feature!

Tue, Jan 12, 7:06 AM · Restricted Project
sammccall accepted D94479: [clangd] Extend hints for gRPC and Protobuf CMake config paths.

(What broke this?)

Tue, Jan 12, 4:53 AM · Restricted Project
sammccall accepted D94477: [clangd] Add main file macros into the main-file index..

Ah, this is a bit ugly... I'm not really happy with our representation of info extracted from the preprocessor, but that's something to tackle another time.

Tue, Jan 12, 4:52 AM · Restricted Project
sammccall accepted D94382: [clangd] Avoid recursion in TargetFinder::add().
Tue, Jan 12, 4:38 AM · Restricted Project

Mon, Jan 11

sammccall added a reviewer for D93873: [clangd] Cache preambles of closed files: kadircet.

Thanks! This is a really good idea, but not without its risks :-) Sorry for not getting to this for a while!

Mon, Jan 11, 8:02 AM · Restricted Project
sammccall accepted D94359: [clangd] Remove ScratchFS from tests.
Mon, Jan 11, 7:10 AM · Restricted Project
sammccall accepted D94411: [clangd] Fix -check mode doesn't respect any tidy configs..
Mon, Jan 11, 7:09 AM · Restricted Project
sammccall added a comment to D94382: [clangd] Avoid recursion in TargetFinder::add().

Doh, I really thought we'd get away without an explicit recursion guard here.
But the example is compelling, so this seems like the right approach. Unfortunately, I think we're going to end up needing to add allocations too...

Mon, Jan 11, 2:14 AM · Restricted Project

Fri, Jan 8

sammccall added a comment to D94265: [clangd] Search compiler in PATH for system include extraction If the compiler is specified by its name, search it in the system PATH instead of the command directory: this is what the build system would do..

Yes, I was getting deja vu reading the description. Thanks though!

Fri, Jan 8, 6:24 AM · Restricted Project
sammccall accepted D94289: [clangd] Add go-to-def metric..
Fri, Jan 8, 6:23 AM · Restricted Project
sammccall added a comment to D94293: [clangd] automatically index STL.

How does this approach work with differing language standards. For example with an old implementation designed for c++14, string_view header won't exist. If the implementation is designed to work with c++17, including that header in c++14 mode will probably be ifdef... Error.

Fri, Jan 8, 6:17 AM · Restricted Project

Thu, Jan 7

sammccall added a comment to D93978: [clangd] Use dirty filesystem when performing cross file tweaks.

Thanks, this looks much better!

Thu, Jan 7, 5:55 AM · Restricted Project
sammccall committed rG213329d7c64f: [clangd] Add server capability advertising hot-reloading of CDBs. (authored by sammccall).
[clangd] Add server capability advertising hot-reloading of CDBs.
Thu, Jan 7, 4:39 AM
sammccall closed D94222: [clangd] Add server capability advertising hot-reloading of CDBs..
Thu, Jan 7, 4:39 AM · Restricted Project
sammccall requested review of D94222: [clangd] Add server capability advertising hot-reloading of CDBs..
Thu, Jan 7, 2:40 AM · Restricted Project

Tue, Jan 5

sammccall added a comment to D93978: [clangd] Use dirty filesystem when performing cross file tweaks.

Ah, thanks for working on this!

Tue, Jan 5, 7:22 AM · Restricted Project
sammccall added a comment to D93452: [clangd] Trim memory periodically.

I tried to sketch a couple of tetris-based mental models for why these hashtables might never fit inside an existing gap, leading us to stack our arena size (tetris field height) higher and higher.
This is predicated on the assumption that the index hashtables are the biggest single contiguous allocations in clangd, which I think is true. (It also explains why std::map fares better - each node is a separate allocation. Unfortunately, this is also why it's performance is terrible...)
Not sure which/either is model true, but I found it entertaining.

Tue, Jan 5, 5:17 AM · Restricted Project, Restricted Project
sammccall added a comment to D93452: [clangd] Trim memory periodically.

It does indeed look like facing the problem from the wrong side (fight the end result instead of finding the root cause) but at that point it does solve an annoying problem. As I investigated this, I found that replacing DenseMap and StringMap wtih std::map mitigates the issue. I think allocating huge chunks of contiguous memory temporarily make this issue worse. Not very useful, but hey, I'm just sharing what I observed. As I previously said, resolving this problem by changing containers or allocators is a lot of work and is not guaranteed to actually solve the issue.

Tue, Jan 5, 4:58 AM · Restricted Project, Restricted Project
sammccall added a comment to D93763: [clangd] Add a flag to disable the documentLinks LSP request.

Seems fair enough to do something about this, though it's a bit sad we're doing it just because VSCode has bad UI.

Tue, Jan 5, 4:25 AM · Restricted Project
sammccall accepted D93796: [clangd][fuzzyFind] Do not show stale symbols in the result..

Yup, as mentioned on D93393 this seems like a reasonable tradeoff to me. Thanks!

Tue, Jan 5, 4:07 AM · Restricted Project
sammccall added a comment to D93600: [clangd] When querying drivers by binary, look in PATH too.

Sorry for the delay getting this landed, and thanks for the patch!
It'll be part of the clangd 12 release.

Tue, Jan 5, 3:55 AM · Restricted Project
sammccall committed rG2f8d1e9eb27e: [clangd] When querying drivers by binary, look in PATH too (authored by rapgenic).
[clangd] When querying drivers by binary, look in PATH too
Tue, Jan 5, 3:54 AM
sammccall closed D93600: [clangd] When querying drivers by binary, look in PATH too.
Tue, Jan 5, 3:54 AM · Restricted Project

Sat, Dec 26

sammccall added a comment to D78058: option to write files to memory instead of disk.

@dexonsmith thanks for sharing!
Some initial thoughts since abstracting outputs is something we're starting to care about too...

Sat, Dec 26, 3:40 AM · Restricted Project

Dec 23 2020

sammccall committed rG74b3acefc7b6: [clangd] Fix case mismatch crash on in CDB on windows after 92dd077af1ff8 (authored by sammccall).
[clangd] Fix case mismatch crash on in CDB on windows after 92dd077af1ff8
Dec 23 2020, 1:43 PM
sammccall added a comment to D93683: [clangd] Do not take stale definition from the static index..

I'm not 100% sure this bug would block adding the equivalent change for fuzzyFind though - it'll affect documentSymbol but not code-completion IIUC (because of the workaround I mentioned above). And these symbols are pretty rare.

With the similar change for fuzzyFind(), WorkspaceSymbols.Macros test fails. But maybe I can create one more review for this and we will discuss it there.

Dec 23 2020, 12:06 AM · Restricted Project

Dec 22 2020

sammccall accepted D93600: [clangd] When querying drivers by binary, look in PATH too.

Thanks, this looks good, just a couple of nits.

Dec 22 2020, 2:20 PM · Restricted Project
sammccall added a comment to D93452: [clangd] Trim memory periodically.

I feel like we need some kind of entry for this in release notes given how much of an issue it is.

Dec 22 2020, 2:00 PM · Restricted Project, Restricted Project
sammccall committed rGf7a26127f21f: [clangd] Release notes for b8c37153d5393aad96 (authored by sammccall).
[clangd] Release notes for b8c37153d5393aad96
Dec 22 2020, 1:59 PM
sammccall committed rG3dbe471a2603: [clangd] Use atomics instead of locks to track periodic memory trimming (authored by sammccall).
[clangd] Use atomics instead of locks to track periodic memory trimming
Dec 22 2020, 1:32 PM
sammccall closed D93726: [clangd] Use atomics instead of locks to track periodic memory trimming.
Dec 22 2020, 1:32 PM · Restricted Project
sammccall added a comment to D93726: [clangd] Use atomics instead of locks to track periodic memory trimming.

LGTM

Small nits:

  • operator() is not const but Next is mutable, seems to me you intended to have operator() as const

Oops, I originally did plan to make operator() const, but then decided it'd be clearer for the caller to make PeriodicThrottler mutable instead. (Because pretending it doesn't have state is confusing).
Removed mutable from Next.

Dec 22 2020, 1:30 PM · Restricted Project
sammccall accepted D93683: [clangd] Do not take stale definition from the static index..

I also want to propose the similar patch for fuzzyFind() (as a part of this patch or a separate one), but I faced the following problem:

Test.cpp

#define FOO 1

This example creates empty dynamic index for main file symbols and static index for preamble symbols (FOO). The location of FOO is inside Test.cpp, so (according to the logic that the static index could be stale) we should toss FOO from the static index (i.e. preamble symbols), but this behaviour is incorrect... Any advice?

Dec 22 2020, 1:09 PM · Restricted Project
sammccall requested review of D93726: [clangd] Use atomics instead of locks to track periodic memory trimming.
Dec 22 2020, 12:36 PM · Restricted Project

Dec 21 2020

sammccall committed rGb8c37153d539: [clangd] Trim memory periodically when using glibc malloc (authored by qchateau).
[clangd] Trim memory periodically when using glibc malloc
Dec 21 2020, 11:55 PM
sammccall closed D93452: [clangd] Trim memory periodically.
Dec 21 2020, 11:54 PM · Restricted Project, Restricted Project
sammccall accepted D93452: [clangd] Trim memory periodically.

I did not use the CMke option as the default value for the command line option: IMO this CMake option is only useful if you encounter build problems related to malloc_trim, so malloc_trim must not be part of the code when you disable it through CMake.

This seems a bit too conservative to me, I wouldn't expect anyone to ever encounter such build problems (e.g. glibc so old that malloc_trim doesn't exist, or header exists but library doesn't), so I'd add a workaround if they actually arose.
The other value of the CMake option is being able to disable the *default* behavior if you use another allocator, so that you don't have to ask all users to pass a custom flag.

Dec 21 2020, 11:43 PM · Restricted Project, Restricted Project
sammccall committed rG3fa2d37eb3f8: [clangd][NFC] Improve clangd status messages (authored by qchateau).
[clangd][NFC] Improve clangd status messages
Dec 21 2020, 11:19 AM
sammccall closed D93546: [clangd][NFC] Improve clangd status messages.
Dec 21 2020, 11:19 AM · Restricted Project
sammccall added a comment to D93546: [clangd][NFC] Improve clangd status messages.

It's up to you - it's easy for me/others to land them on your behalf. Having your own commit access gives you more control, ability to deal with build breakages quickly, fix trivial things without review etc.

Dec 21 2020, 11:19 AM · Restricted Project
sammccall added a comment to D93531: [clangd] Reuse buffer for JSONTransport::sendMessage.

I've put a version of the reading part in D93653. This seems a bit less invasive to me - I'm still not sure if it's worth the readability hit though...

Dec 21 2020, 11:16 AM · Restricted Project
sammccall requested review of D93653: [clangd] Avoid reallocating buffers for each message read:.
Dec 21 2020, 11:13 AM · Restricted Project
sammccall accepted D93531: [clangd] Reuse buffer for JSONTransport::sendMessage.

Thanks, this version seems just as clear as the original to me, and fewer allocations is nice :-)

Dec 21 2020, 10:42 AM · Restricted Project
sammccall accepted D93546: [clangd][NFC] Improve clangd status messages.

Thanks, LG - want me to land it?

Dec 21 2020, 10:39 AM · Restricted Project
sammccall added a comment to D93600: [clangd] When querying drivers by binary, look in PATH too.

Thanks, this looks like a good idea to me, but some quibbles with the details:

  • for any string, we statically know whether it's path or WD-relative
  • we should make sure all the IO gets cached
Dec 21 2020, 10:39 AM · Restricted Project
sammccall accepted D93452: [clangd] Trim memory periodically.

Awesome! Some nits on naming and stuff, but the only real substantial question for me is the exact availability/behavior of the command-line flag. Even there, as long as the default behavior is to trim when build with glibc, the details are less important.
Let me know when you want me to land it!

Dec 21 2020, 10:19 AM · Restricted Project, Restricted Project

Dec 20 2020

sammccall added a comment to D93452: [clangd] Trim memory periodically.

Sorry to mess you around like this, but I think I might have jumped the gun in asking this to be abstracted away in Process.h. My reasoning is:

  • Once we are injecting the cleanup function from ClangdMain, that's not actually a crazy place to put this kind of system-level code, behind #ifdefs
  • The abstraction costs us something: we can't skip all the locking etc for memory cleanups if it's a no-op on our platform
  • We're putting an abstraction around one implementation without any reason to believe other implementations need this treatment or can use a similar interface. This is likely to turn out to be a poor abstraction, and llvm/Support isn't the easiest place to iterate on it.
Dec 20 2020, 5:16 AM · Restricted Project, Restricted Project

Dec 18 2020

sammccall committed rG2b62e6232884: [clangd] Fix windows path handling in .clang-tidy parsing (authored by sammccall).
[clangd] Fix windows path handling in .clang-tidy parsing
Dec 18 2020, 5:24 PM
sammccall added a comment to D93546: [clangd][NFC] Improve clangd status messages.

Thanks! Changes to ClangdServer are definitely good, I'm less sure about renderTUAction.

Dec 18 2020, 5:11 PM · Restricted Project
sammccall committed rG2fced5a07b45: [clangd] Don't cancel requests based on "updates" with same content (authored by sammccall).
[clangd] Don't cancel requests based on "updates" with same content
Dec 18 2020, 5:04 PM
sammccall committed rGb0615642f647: [clangd] Make our printing policies for Hover more consistent, especially tags (authored by sammccall).
[clangd] Make our printing policies for Hover more consistent, especially tags
Dec 18 2020, 4:03 PM
sammccall closed D93553: [clangd] Make our printing policies for Hover more consistent, especially tags.
Dec 18 2020, 4:03 PM · Restricted Project
sammccall added a comment to D93553: [clangd] Make our printing policies for Hover more consistent, especially tags.

Looks good to me, it removes a lot of duplication and corner cases.

I'd have printed the tag for reference and pointers as well (class Foo* instead of Foo*). I don't think Foo* is much more understandable than Foo, so we decide that printing class Foo is easier to understand for the user, we should print class Foo* as well.
But I agree it's even less idiomatic, so I won't argue with your choice.

Dec 18 2020, 3:51 PM · Restricted Project
sammccall added a comment to D93452: [clangd] Trim memory periodically.

Thanks, this looks much closer to something we can land.
Bunch of nits :-)

Dec 18 2020, 3:39 PM · Restricted Project, Restricted Project
sammccall added a comment to D93531: [clangd] Reuse buffer for JSONTransport::sendMessage.

This adds a bit of complexity, making the code here a fair amount harder to follow and verify the correctness of.

  • Do we have evidence that these allocations are causing a problem? (e.g. do we observe a significant decrease in RSS after the patch)? Naively I would expect these allocations to be basically unimportant compared to those of the JSON objects themselves.(And I don't particularly expect either of them to be significant - the comment on the other review was really just "0 probably isn't the right arg to malloc_trim if there's any allocation going on").

It's not causing problems per say. but given the incoming json messages can contain a whole file plus things like escape chars. its wise to allocate a buffer that will grow to the largest json it receives but never shrink.

Dec 18 2020, 2:30 PM · Restricted Project