sammccall (Sam McCall)Administrator
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

sammccall accepted D46943: [clangd] Boost scores for decls from current file in completion.

Nice, simple and will admit refinements later.

Thu, May 24, 9:04 AM
sammccall added a comment to D47063: [clangd] Keep only a limited number of idle ASTs in memory.

Thanks, this looks a lot better!
There do seem to be a lot of little classes that exist exactly one-per-TU (ASTWorker, ASTBuilder, CachedAST, to a lesser extent ParsedAST) and I'm not sure the balance of responsibilities is quite right. Some comments below.

Thu, May 24, 8:00 AM
sammccall accepted D47274: [clangd] Serve comments for headers decls from dynamic index only.
Thu, May 24, 6:10 AM
sammccall added a comment to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

@chandlerc: Back-from-vacation ping.
(A short response like "yes, do change X" is fine, keeps me busy :-)

Thu, May 24, 1:44 AM
sammccall added inline comments to D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type.
Thu, May 24, 1:12 AM · Restricted Project

Yesterday

sammccall added a comment to D47063: [clangd] Keep only a limited number of idle ASTs in memory.

Having taken a closer look, I think the cache can be simplified/separated a bit more cleanly by returning shared pointers and not allowing lookups, instead restoring limited ownership in CppFile...

Wed, May 23, 9:49 AM
sammccall added inline comments to D47187: [clangd] Skip .inc headers when canonicalizing header #include..
Wed, May 23, 7:50 AM
sammccall added a comment to D47063: [clangd] Keep only a limited number of idle ASTs in memory.

(Haven't reviewed the code yet, just design stuff)
I'm tempted to push back on the idea that we need to store any - "if it's fast enough for code complete, it's fast enough for GTD". But that's probably oversimplifying - we don't force reuse of a stable preamble for GTD I think. So we probably do want some cache.

Wed, May 23, 7:38 AM
sammccall accepted D47257: [clang-format] Break template declarations followed by comments.

Thanks for chasing this, good detective work!
Changes look plausible and tests are nice, so LG assuming you know what you're doing. If you're unsure, Manuel will give you better advice than me :)

Wed, May 23, 7:06 AM
sammccall accepted D47187: [clangd] Skip .inc headers when canonicalizing header #include..

n

Wed, May 23, 2:50 AM

Wed, May 16

sammccall added inline comments to D46943: [clangd] Boost scores for decls from current file in completion.
Wed, May 16, 9:26 AM

Tue, May 15

sammccall committed rCTE332378: [clangd] Extract scoring/ranking logic, and shave yaks..
[clangd] Extract scoring/ranking logic, and shave yaks.
Tue, May 15, 10:47 AM
sammccall committed rL332378: [clangd] Extract scoring/ranking logic, and shave yaks..
[clangd] Extract scoring/ranking logic, and shave yaks.
Tue, May 15, 10:47 AM
sammccall closed D46524: [clangd] Extract scoring/ranking logic, and shave yaks..
Tue, May 15, 10:47 AM
sammccall added a comment to D46751: [clangd] Filter out private proto symbols in SymbolCollector..

@malaperle to expand on what Eric said, adding proto hacks with false positives and no way to turn them off is indeed not the way to go here!
There's probably going to be other places we want to filter symbols too, and it should probably be extensible/customizable in some way.
We don't yet have enough examples to know what the structure should be (something regex based, a code-plugin system based on Registry, or something in between), thus the simplest/least invasive option for now (it's important for our internal rollout to have *some* mitigation in place).
@ioeric can you add a comment near the proto-filtering stuff indicating we should work out how to make this extensible?

I agree with all of that. What I don't quite understand is why a flag is not ok? Just a fail-safe switch in the mean time? You can even leave it on by default so your internal service is not affected.

I think a flag doesn't solve much of the problem, and adds new ones:

  • users have to find the flag, and work out how to turn it on in their editor, and (other than embedders) few will bother. And each flag hurts the usability of all the other flags.
  • if this heuristic is usable only sometimes, that's at codebase granularity, not user granularity. Flags don't work that way. (Static index currently has this problem...)
  • these flags end up in config files, so if we later remove the flag we'll *completely* break clangd for such users
Tue, May 15, 9:56 AM
sammccall added inline comments to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..
Tue, May 15, 9:32 AM
sammccall updated the diff for D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

Array wraps vector instead of inheriting it.
Object inherits DenseMap instead of std::map
Change some Object accessors to use StringRef instead of ObjectKey.
clang-format, because it was getting out of control.

Tue, May 15, 9:27 AM
sammccall added a comment to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

(Looks like Phabricator swallowed most of my email reply, please excuse some repetition)

Tue, May 15, 9:25 AM
sammccall edited reviewers for D43667: [clang-doc] Implement a YAML generator, added: ioeric; removed: sammccall.
Tue, May 15, 7:20 AM · Restricted Project
sammccall edited reviewers for D43424: [clang-doc] Implement a (simple) Markdown generator, added: ioeric; removed: sammccall.
Tue, May 15, 7:20 AM · Restricted Project
sammccall added a comment to D46751: [clangd] Filter out private proto symbols in SymbolCollector..

@malaperle to expand on what Eric said, adding proto hacks with false positives and no way to turn them off is indeed not the way to go here!
There's probably going to be other places we want to filter symbols too, and it should probably be extensible/customizable in some way.
We don't yet have enough examples to know what the structure should be (something regex based, a code-plugin system based on Registry, or something in between), thus the simplest/least invasive option for now (it's important for our internal rollout to have *some* mitigation in place).

Tue, May 15, 5:00 AM
sammccall accepted D46497: [clangd] Populate #include insertions as additional edits in completion items..

Nice, ship it!

Tue, May 15, 4:52 AM

Mon, May 14

sammccall added inline comments to D46497: [clangd] Populate #include insertions as additional edits in completion items..
Mon, May 14, 6:35 AM
sammccall updated the diff for D46524: [clangd] Extract scoring/ranking logic, and shave yaks..

Doxygen

Mon, May 14, 6:33 AM
sammccall updated the diff for D46524: [clangd] Extract scoring/ranking logic, and shave yaks..

fix diffbase?

Mon, May 14, 5:32 AM
sammccall accepted D46675: [clangd] Add helper for collecting #include directives in file..
Mon, May 14, 4:44 AM
sammccall accepted D46001: [CodeComplete] Expose helpers to get RawComment of completion result..

This is pretty small and seems unlikely to be controversial. I think it's OK to commit at this point, it's been a few weeks.

Mon, May 14, 1:32 AM
sammccall accepted D45999: [clangd] Retrieve minimally formatted comment text in completion..

Looks good, nits as always and I think you want a new test case.

Mon, May 14, 1:25 AM
sammccall accepted D46795: [clangd] Don't query index when completing inside classes.
Mon, May 14, 1:02 AM

Thu, May 10

sammccall updated subscribers of D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

Thanks for the thoughts, I'll take another pass and try to incorporate
them. But I'm out until Monday, so some answers/questions while this is in
your cache...

Thu, May 10, 1:32 PM

Mon, May 7

sammccall added inline comments to D46524: [clangd] Extract scoring/ranking logic, and shave yaks..
Mon, May 7, 9:02 AM
sammccall updated the diff for D46524: [clangd] Extract scoring/ranking logic, and shave yaks..

Address comments

Mon, May 7, 9:02 AM
sammccall added a comment to D46497: [clangd] Populate #include insertions as additional edits in completion items..

I'm concerned about the scope of this patch - it does too many things and touches too many files for me to feel comfortable that I understand it well enough to review.
Is it possible to split it up? (You mention 6 distinct things in the description).

Mon, May 7, 8:19 AM
sammccall created D46524: [clangd] Extract scoring/ranking logic, and shave yaks..
Mon, May 7, 6:49 AM
sammccall added a comment to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

@chandlerc Ping - I think you're the best person to decide whether a JSON-specific library should be added.
It'd be great to make some progress on that, even if actual review would take longer or be left for others.
(I think simon_tatham has work blocked on this, and there are some clangd changes I'm holding off to avoid diverging)
Are the arguments above compelling? Any more info I can provide?

Mon, May 7, 4:39 AM
sammccall accepted D46176: Add SourceManagerForFile helper which sets up SourceManager and dependencies for a single file with code snippet.
Mon, May 7, 1:42 AM

Thu, May 3

sammccall committed rL331457: [clangd] Incorporate #occurrences in scoring code complete results..
[clangd] Incorporate #occurrences in scoring code complete results.
Thu, May 3, 7:56 AM
sammccall committed rCTE331457: [clangd] Incorporate #occurrences in scoring code complete results..
[clangd] Incorporate #occurrences in scoring code complete results.
Thu, May 3, 7:56 AM
sammccall closed D46183: [clangd] Incorporate #occurrences in scoring code complete results..
Thu, May 3, 7:56 AM
sammccall updated the diff for D46183: [clangd] Incorporate #occurrences in scoring code complete results..

Fix tests.

Thu, May 3, 7:46 AM
sammccall added a comment to D46183: [clangd] Incorporate #occurrences in scoring code complete results..

Added a test - it's kind of perfunctory, I really want to organize symbol scoring more thoroughly which will provide a better place to test well.

Thu, May 3, 7:14 AM
sammccall updated the diff for D46183: [clangd] Incorporate #occurrences in scoring code complete results..

Add completion test.

Thu, May 3, 7:13 AM

Wed, May 2

sammccall added inline comments to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..
Wed, May 2, 11:54 PM
sammccall added a comment to D46274: [Support] Harden JSON against invalid UTF-8..

Is it super tricky to at least extract the UTF-8 validating and sanitizing functions into their own header and source files within the Support library, if not a separate library?

Wed, May 2, 7:06 AM
sammccall updated the diff for D46274: [Support] Harden JSON against invalid UTF-8..

Use existing library code from ConvertUTF for UTF-8 validation.

Wed, May 2, 7:02 AM
sammccall updated the diff for D46274: [Support] Harden JSON against invalid UTF-8..

Reject invalid codepoints (surrogates, high codepoints).
Add more comments around UTF8 validation.

Wed, May 2, 6:19 AM
sammccall added inline comments to D46176: Add SourceManagerForFile helper which sets up SourceManager and dependencies for a single file with code snippet.
Wed, May 2, 5:21 AM
sammccall added inline comments to D43341: [clang-doc] Implement reducer portion of the frontend framework.
Wed, May 2, 1:47 AM · Restricted Project

Tue, May 1

sammccall added a comment to D42966: Fix USR generation in the presence of #line directives or linemarkes.

Hi,

After thinking about this a bit, and use cases like rename and
find-declaration that could be USR based, I think including some location
information is the right way to go, which I think is the current behavior.

What do you man by location information? Only the filename or filename +
offset (current behaviour)?

Tue, May 1, 11:38 PM

Mon, Apr 30

sammccall updated subscribers of D46274: [Support] Harden JSON against invalid UTF-8..

Thanks for the comments!
Not validating codepoints was deliberate but in hindsight arbitrary, I'll
fix that. (Both surrogates and high codepoints).

Mon, Apr 30, 11:22 AM
sammccall added a comment to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

@chandlerc This is ready for you again whenever you have cycles. To summarize:

  • I think everything above was resolved except for the question of whether the mutable/initializer_list tricks are too gross to use.
  • a few more potential users seem pleased about this idea, and @simon_tatham has verified that it works for tablegen in D46054.
  • I've had a couple of suggestions which have been moved into D46209 (numeric precision) and D46274 (unicode validation), I don't think you personally need to review those.
Mon, Apr 30, 10:44 AM
sammccall updated the diff for D46209: [Support] Make JSON handle doubles and int64s losslessly.

Negative number test case, fix std:: qualification.

Mon, Apr 30, 10:33 AM
sammccall added a comment to D46209: [Support] Make JSON handle doubles and int64s losslessly.

Thanks for verifying this! (Still waiting for a decision on the master patch, I hope this doesn't block you too long!)

Mon, Apr 30, 10:33 AM
sammccall added a dependent revision for D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support.: D46274: [Support] Harden JSON against invalid UTF-8..
Mon, Apr 30, 10:29 AM
sammccall added a dependency for D46274: [Support] Harden JSON against invalid UTF-8.: D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..
Mon, Apr 30, 10:29 AM
sammccall created D46274: [Support] Harden JSON against invalid UTF-8..
Mon, Apr 30, 10:26 AM
sammccall added inline comments to D46035: [clangd] Fix unicode handling, using UTF-16 where LSP requires it..
Mon, Apr 30, 9:25 AM
sammccall added inline comments to D46035: [clangd] Fix unicode handling, using UTF-16 where LSP requires it..
Mon, Apr 30, 9:16 AM
sammccall accepted D45717: [clangd] Using index for GoToDefinition..

A few more nits (and some ideas for further restructuring the merge logic).
Otherwise LG, let's land this!

Mon, Apr 30, 6:50 AM
sammccall accepted D46258: [clangd] Also use UTF-16 in index position..

(oops, meant to accept this - please do add a test if you can)

Mon, Apr 30, 3:18 AM
sammccall added a comment to D46258: [clangd] Also use UTF-16 in index position..

Oops, I missed this, thanks!
Possible to add a testcase? Any character that's not ASCII and is legal in an identifier should do it.
http://en.cppreference.com/w/cpp/language/identifiers

Mon, Apr 30, 3:18 AM
sammccall added inline comments to D46035: [clangd] Fix unicode handling, using UTF-16 where LSP requires it..
Mon, Apr 30, 1:42 AM

Fri, Apr 27

sammccall added inline comments to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..
Fri, Apr 27, 2:13 PM
sammccall updated the diff for D46209: [Support] Make JSON handle doubles and int64s losslessly.

Clarify docs, add deserializer for int64_t

Fri, Apr 27, 1:32 PM
sammccall added a dependency for D46209: [Support] Make JSON handle doubles and int64s losslessly: D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..
Fri, Apr 27, 1:20 PM
sammccall added a dependent revision for D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support.: D46209: [Support] Make JSON handle doubles and int64s losslessly.
Fri, Apr 27, 1:20 PM
sammccall created D46209: [Support] Make JSON handle doubles and int64s losslessly.
Fri, Apr 27, 1:18 PM
sammccall added inline comments to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..
Fri, Apr 27, 6:31 AM
sammccall updated the diff for D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

Fix GCC warnings/errors.
Add note about integer representation.

Fri, Apr 27, 6:30 AM
sammccall created D46183: [clangd] Incorporate #occurrences in scoring code complete results..
Fri, Apr 27, 5:33 AM
sammccall committed rCTE331029: [clangd] Fix unicode handling, using UTF-16 where LSP requires it..
[clangd] Fix unicode handling, using UTF-16 where LSP requires it.
Fri, Apr 27, 5:04 AM
sammccall committed rL331029: [clangd] Fix unicode handling, using UTF-16 where LSP requires it..
[clangd] Fix unicode handling, using UTF-16 where LSP requires it.
Fri, Apr 27, 5:03 AM
sammccall closed D46035: [clangd] Fix unicode handling, using UTF-16 where LSP requires it..
Fri, Apr 27, 5:02 AM
sammccall accepted D46002: [clangd] Parse all comments in Sema and completion..

LG (once the dependencies are done!)

Fri, Apr 27, 5:02 AM
sammccall added a comment to D46035: [clangd] Fix unicode handling, using UTF-16 where LSP requires it..

Thanks!

Fri, Apr 27, 4:33 AM
sammccall added a comment to D45999: [clangd] Retrieve minimally formatted comment text in completion..

My main question/concern is: if these APIs extract/format based on CodeCompletionString and friends, what should our plan be using this this in AST-based features, such as hover?

Fri, Apr 27, 4:17 AM
sammccall added a comment to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

Looks like I forgot to clang-format before sending this :-/

Fri, Apr 27, 2:53 AM
sammccall updated the diff for D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

Doxygen, comment and error message tweaks.

Fri, Apr 27, 2:51 AM
sammccall added a comment to D42966: Fix USR generation in the presence of #line directives or linemarkes.

Re: locations in parameter USRs:

Fri, Apr 27, 2:07 AM

Wed, Apr 25

sammccall added a comment to D46002: [clangd] Parse all comments in Sema and completion..

Code looks good/simple, just a couple of design questions

Wed, Apr 25, 2:59 AM

Tue, Apr 24

sammccall updated the diff for D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

Address some review comments (but no doxygen yet)

Tue, Apr 24, 7:57 PM
sammccall added a comment to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

@chandlerc: any interest in reviewing this JSON library (either at a high level for suitability in llvm/Support, or for detailed review)?

I'm happy to tackle both of these. I'll want to do a bit of homework to make sure that the high level concerns that came up previously are adequately addressed (or there is some plan to address them).

Thank you! Other than "how does this relate to YAML", I don't think I'm familiar with the concerns so any pointers you have would be appreciated.

Tue, Apr 24, 7:56 PM
sammccall updated subscribers of D44882: [clangd] Implementation of workspace/symbol request.

So this fails if there's no standard library available without flags, which is the case in google's test environment to ensure hermeticity :-(

In the short-term, we've disabled the test internally - did it trigger any buildbot failures?
In the medium term we should probably find a better way to test this :(

No buildbot failures yet. But I suggest we just remove the test and add it back when we collect more symbols, i.e. in main files. WDYT?

Tue, Apr 24, 4:40 PM
sammccall updated the diff for D46035: [clangd] Fix unicode handling, using UTF-16 where LSP requires it..

Remove some debugging junk, tweak a comment.

Tue, Apr 24, 4:36 PM
sammccall updated the diff for D46035: [clangd] Fix unicode handling, using UTF-16 where LSP requires it..

clang-format

Tue, Apr 24, 4:27 PM
sammccall created D46035: [clangd] Fix unicode handling, using UTF-16 where LSP requires it..
Tue, Apr 24, 4:27 PM
sammccall committed rL330762: [Support] fix countLeadingZeros for types shorter than int.
[Support] fix countLeadingZeros for types shorter than int
Tue, Apr 24, 1:11 PM

Apr 24 2018

sammccall added a comment to D44882: [clangd] Implementation of workspace/symbol request.

Still LG, thanks!
I'll look into the testing issue.

I thought about it after... I think it was because I was trying to test with std::unordered_map (to prevent multiple results) which needs std=c++11, I'll try with something else.

Worth a shot, but don't count on it - for c++ clang switched to using std=c++11 by default a while ago.

I just tried "std::basic_ostringstream" and that works. Seems safer.

Apr 24 2018, 8:46 AM
sammccall accepted D45887: [CodeComplete] Fix completion at the end of keywords.

Nice! I'm a little surprised that this wasn't fixed before, so I'm worried I missing something.
But nobody else has reviewed it, and it really seems right (and the test cases you fixed did seem broken). We can always revert :)

Apr 24 2018, 2:04 AM

Apr 23 2018

sammccall added a comment to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

@chandlerc: any interest in reviewing this JSON library (either at a high level for suitability in llvm/Support, or for detailed review)?

Apr 23 2018, 10:39 AM
sammccall added a reviewer for D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support.: chandlerc.
Apr 23 2018, 10:35 AM
sammccall updated the diff for D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

Address review comments, add a few more docs.
Go back to custom UTF transcoding because of failing test cases :-(

Apr 23 2018, 10:00 AM
sammccall added a comment to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

This looks good to me, but I do feel someone else should comment on the appropriateness of including this library in llvm/Support. Chandler is listed as the code owner of Support, and he used to have opinions on json parsers in the past, so maybe he would be a good candidate (?)

Apr 23 2018, 9:59 AM
sammccall added a comment to D45717: [clangd] Using index for GoToDefinition..

Thanks for the changes! I still find the logic quite confusing. Happy to chat offline if useful.

Apr 23 2018, 7:14 AM

Apr 20 2018

sammccall added a comment to D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

I'm not sure who should be the main person reviewing this,

I'm also not sure, but I really appreciate your feedback, and wanted to give you as a likely user a chance to shape the API.
Thanks for the great advice, don't feel any pressure to accept or examine everything if you don't feel like the right person.

Apr 20 2018, 10:06 AM
sammccall updated the diff for D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support..

Address review comments.

Apr 20 2018, 10:06 AM
sammccall committed rCTE330418: Parse .h files as objective-c++ if we don't have a compile command..
Parse .h files as objective-c++ if we don't have a compile command.
Apr 20 2018, 4:39 AM
sammccall committed rL330418: Parse .h files as objective-c++ if we don't have a compile command..
Parse .h files as objective-c++ if we don't have a compile command.
Apr 20 2018, 4:38 AM
sammccall closed D45442: Parse .h files as objective-c++ if we don't have a compile command..
Apr 20 2018, 4:38 AM

Apr 19 2018

sammccall added a comment to D45478: [clangd] Merge symbols in global-sym-builder on the go.

I think this implementation will have problems in a "real" multi-machine MR framework. The lifetime of the Merger is the whole program, with output only coming at the end.
With N workers, each will store >1/N of the symbols (more because of overlap), and the streaming nature of a MR is important to its scaling.

Apr 19 2018, 8:48 AM