sammccall (Sam McCall)Administrator
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Sun, Aug 12

sammccall accepted D50608: [Support][JSON][NFC] Silence GCC warning about broken strict aliasing rules.

I also prefer this version over suppressing the warning with the pragma.
Thanks!

Sun, Aug 12, 8:49 AM

Wed, Aug 8

sammccall planned changes to D50439: [Tooling] Switch JSONCompilationDatabase to use JSON parser..

Nevermind, this is like 5x slower to load the chromium CDB. probably because of all the little non-arena allocations.
I'll profile and fix it when I get bored again.

Wed, Aug 8, 5:50 AM
sammccall updated the summary of D50439: [Tooling] Switch JSONCompilationDatabase to use JSON parser..
Wed, Aug 8, 5:49 AM
sammccall updated the diff for D50439: [Tooling] Switch JSONCompilationDatabase to use JSON parser..

(just updating description)

Wed, Aug 8, 5:31 AM
sammccall created D50439: [Tooling] Switch JSONCompilationDatabase to use JSON parser..
Wed, Aug 8, 5:28 AM

Tue, Aug 7

sammccall updated subscribers of D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested).

Couple of thoughts. (Technically I'm out on leave so will let Jan/Ilya
review implementation and happy with whatever you decide)

Tue, Aug 7, 6:43 PM

Thu, Aug 2

sammccall added a comment to D50154: [clangd] capitalize diagnostic messages.

Capitalizing sounds nice. +1 to just do it without an option.

Thu, Aug 2, 5:57 AM

Sun, Jul 29

sammccall accepted D49967: [clangd] Add command-line option to suppress the space and the circular dot prepended in a completion label..

Makes sense, thanks!

Sun, Jul 29, 11:40 AM

Sat, Jul 28

sammccall accepted D49950: [Polly][JSONExporter] Replace bundled Jsoncpp with LLVM's JSON.h. NFC..

Usage of JSON.h looks pretty good, thanks for doing this migration!

Sat, Jul 28, 2:58 AM · Restricted Project

Tue, Jul 24

sammccall committed rC337834: [VFS] Cleanups to VFS interfaces..
[VFS] Cleanups to VFS interfaces.
Tue, Jul 24, 9:01 AM
sammccall committed rL337834: [VFS] Cleanups to VFS interfaces..
[VFS] Cleanups to VFS interfaces.
Tue, Jul 24, 9:01 AM
sammccall closed D49724: [VFS] Cleanups to VFS interfaces..
Tue, Jul 24, 9:01 AM
sammccall added a comment to D49267: [clangd] Watch for changes in compile_commands.json.

Just a couple of high-level comments here:

  • I'm not sure we can/should commit to supporting editor-based file watching forever.
    • One natural long-term direction would be to get this functionality into JSONCompilationDatabase, and clients of that don't have an LSP client to provide events
    • client support is going to remain uneven in the foreseeable future. LSP is as always underspecified here, and there's no chance that every editor plugin is going to do a reasonable job of recursive file watching, even among those that advertise support for it.
    • if/when we have a native implementation, supporting multiple mechanisms with different layering requirements to get at most a 2x win in watcher resource usage seems like a dubious way to spend our complexity budget.
  • it looks like the current design throws away all compilation databases (via the global DB) when *any* compile_commands.json changes. I think it's important that other stateful compilation databases aren't thrown away for unrelated reasons. e.g. the Bazel DB takes nontrivial time to reinitialize.
Tue, Jul 24, 7:58 AM
sammccall accepted D49591: [clangd] Introduce Dex symbol index search tokens.

Thanks for the polish!
Last few nits, but feel free to land once you're happy.

Tue, Jul 24, 7:13 AM · Restricted Project
sammccall added a comment to D49546: [clangd] Proof-of-concept query iterators for Dex symbol index.

This looks really nice.
Iterator implementations can be simplified a bit I think.

Tue, Jul 24, 7:04 AM · Restricted Project
sammccall created D49724: [VFS] Cleanups to VFS interfaces..
Tue, Jul 24, 4:34 AM
sammccall updated the diff for D49719: [ClangFormat] Editor integrations inherit default style from clang-format binary.

Remove one last -style flag.

Tue, Jul 24, 12:57 AM
sammccall added a comment to D49719: [ClangFormat] Editor integrations inherit default style from clang-format binary.

Thanks, good catch!

Tue, Jul 24, 12:57 AM
sammccall edited reviewers for D49719: [ClangFormat] Editor integrations inherit default style from clang-format binary, added: ilya-biryukov; removed: ioeric.
Tue, Jul 24, 12:01 AM
sammccall created D49719: [ClangFormat] Editor integrations inherit default style from clang-format binary.
Tue, Jul 24, 12:00 AM

Mon, Jul 23

sammccall accepted D49667: [clangd] Tune down quality score for class constructors so that it's ranked after class types..

No opinion on the boost style thing.

Mon, Jul 23, 9:27 AM
sammccall added a comment to D49591: [clangd] Introduce Dex symbol index search tokens.

Looks really nice! Only major issue is the query trigrams don't look right.
Otherwise, some style nits and fixes that seem to have gotten lost.

Mon, Jul 23, 9:24 AM · Restricted Project
sammccall updated subscribers of D49657: [clangd] Make SymbolLocation => bool conversion explicitly..

Or make operator bool explicit

Mon, Jul 23, 4:57 AM
sammccall committed rC337682: [Tooling] Use UniqueStringSaver. NFC.
[Tooling] Use UniqueStringSaver. NFC
Mon, Jul 23, 4:25 AM
sammccall committed rL337682: [Tooling] Use UniqueStringSaver. NFC.
[Tooling] Use UniqueStringSaver. NFC
Mon, Jul 23, 4:25 AM
sammccall added a comment to D49658: [clangd] Index Interfaces for Xrefs.

Mostly LG.

Mon, Jul 23, 3:50 AM
sammccall committed rL337677: [Support] Add a UniqueStringSaver: like StringSaver, but deduplicating..
[Support] Add a UniqueStringSaver: like StringSaver, but deduplicating.
Mon, Jul 23, 3:45 AM
sammccall closed D49596: [Support] Add a UniqueStringSaver: like StringSaver, but deduplicating..
Mon, Jul 23, 3:44 AM
sammccall added a comment to D49546: [clangd] Proof-of-concept query iterators for Dex symbol index.

A few more comments about the bits I understand, but waiting mostly on the documentation.

Mon, Jul 23, 3:32 AM · Restricted Project
sammccall added a comment to D48071: [clangd] Add an option controlling caching of compile commands..

A recent change (D49267) is another indication that caching might be doing more wrong than good. I assume the caching does not give us much performance-wise, we only request compile commands for file reparses and reparses tend to be slow and do lots of file system accesses anyway.
Maybe we should just disable it altogether. @sammccall, @simark, WDYT?

Mon, Jul 23, 1:27 AM

Fri, Jul 20

sammccall added a comment to D48559: [clangd] refactoring for XPC transport layer [NFCI].

Sorry for the delay here, and I'm sorry I haven't been into the patches in detail again yet - crazy week. After experiments, I am convinced the Transport abstraction patch can turn into something nice if we want XPC vs JSON to be a pure transport-level difference with the same semantics. But you need to decide the approach here based on your requirements.

Fri, Jul 20, 10:13 AM · Restricted Project
sammccall accepted D49543: [clangd] Penalize non-instance members when accessed via class instances..
Fri, Jul 20, 9:24 AM
sammccall added inline comments to D49596: [Support] Add a UniqueStringSaver: like StringSaver, but deduplicating..
Fri, Jul 20, 7:28 AM
sammccall updated the diff for D49596: [Support] Add a UniqueStringSaver: like StringSaver, but deduplicating..

Simplify insertion.

Fri, Jul 20, 7:28 AM
sammccall created D49596: [Support] Add a UniqueStringSaver: like StringSaver, but deduplicating..
Fri, Jul 20, 6:22 AM
sammccall committed rL337541: Revert "[LSV] Refactoring + supporting bitcasts to a type of different size".
Revert "[LSV] Refactoring + supporting bitcasts to a type of different size"
Fri, Jul 20, 5:08 AM
sammccall added a comment to rL337489: [LSV] Refactoring + supporting bitcasts to a type of different size.

We're hitting assertions after this revision when running TensorFlow tests
(specifically this test on GPU: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/tests/gather_test.py)

Fri, Jul 20, 5:06 AM
sammccall added inline comments to D49591: [clangd] Introduce Dex symbol index search tokens.
Fri, Jul 20, 4:42 AM · Restricted Project
sammccall added inline comments to D49591: [clangd] Introduce Dex symbol index search tokens.
Fri, Jul 20, 3:34 AM · Restricted Project
sammccall committed rCTE337527: [clangd] FuzzyMatch exposes an API for its word segmentation. NFC.
[clangd] FuzzyMatch exposes an API for its word segmentation. NFC
Fri, Jul 20, 1:09 AM
sammccall committed rL337527: [clangd] FuzzyMatch exposes an API for its word segmentation. NFC.
[clangd] FuzzyMatch exposes an API for its word segmentation. NFC
Fri, Jul 20, 1:07 AM
sammccall closed D49540: [clangd] FuzzyMatch exposes an API for its word segmentation. NFC.
Fri, Jul 20, 1:06 AM

Jul 19 2018

sammccall added a comment to D49546: [clangd] Proof-of-concept query iterators for Dex symbol index.

Thanks for sending this early!
Rough interface comments - mostly looks good though!

Jul 19 2018, 7:55 AM · Restricted Project
sammccall created D49540: [clangd] FuzzyMatch exposes an API for its word segmentation. NFC.
Jul 19 2018, 5:28 AM
sammccall added a comment to D49417: [clangd] Implement trigram generation algorithm for new symbol index.

(just .h files. +1 to eric's comments except where noted)

Jul 19 2018, 3:03 AM
sammccall added a comment to D49417: [clangd] Implement trigram generation algorithm for new symbol index.

Addressed all comments submitted by Eric.

As discussed internally, I should also exercise my naming skills and come up with a better for the symbol index to substitute "Noctem" which doesn't point to any project's feature.

Jul 19 2018, 2:39 AM

Jul 17 2018

sammccall added a comment to D48559: [clangd] refactoring for XPC transport layer [NFCI].

Hi Sam, thanks for your feedback!

In general I agree that explicitly factoring out the transport layer and improving layering would be a good thing. Unfortunately it's highly probable that we'd like to drop JSON completely from XPC dispatch (XPC -> JSON -> ProtocolCallbacks -> JSON -> XPC) in not too distant future. (Please don't take this as a promise as our priorities might change but it's probable enough that I don't recommend to base any common abstraction on JSON.) I originally tried to create similar abstraction but ultimately gave up as it was way too complex. Not saying it's impossible or that I would not like to though!

Jul 17 2018, 11:39 AM · Restricted Project

Jul 16 2018

sammccall added a comment to D48559: [clangd] refactoring for XPC transport layer [NFCI].

Hi Jan,

Jul 16 2018, 11:39 AM · Restricted Project
sammccall created D49389: [clangd] Transport abstraction WIP.
Jul 16 2018, 11:29 AM
sammccall added a comment to D48562: [clangd] XPC transport layer.

Just an initial couple of thoughts here, haven't yet been through in detail.

Jul 16 2018, 3:06 AM · Restricted Project
sammccall accepted D48560: [clangd] JSON <-> XPC conversions.

Sorry it took so long for these to get attention.
Starting here because it's simple and helps me understand the bigger patches....
This one looks good, really just nits.

Jul 16 2018, 2:37 AM · Restricted Project

Jul 13 2018

sammccall committed rCTE336992: [clang-tidy] readability-inconsistent-declaration-parameter-name: accept….
[clang-tidy] readability-inconsistent-declaration-parameter-name: accept…
Jul 13 2018, 4:47 AM
sammccall committed rL336992: [clang-tidy] readability-inconsistent-declaration-parameter-name: accept….
[clang-tidy] readability-inconsistent-declaration-parameter-name: accept…
Jul 13 2018, 4:47 AM
sammccall closed D49285: [clang-tidy] readability-inconsistent-declaration-parameter-name: accept approximate name matches..
Jul 13 2018, 4:47 AM
sammccall updated the diff for D49285: [clang-tidy] readability-inconsistent-declaration-parameter-name: accept approximate name matches..

Restrict matches to prefix/suffix, not substring.

Jul 13 2018, 3:42 AM
sammccall added a comment to D49285: [clang-tidy] readability-inconsistent-declaration-parameter-name: accept approximate name matches..

(First foray into clang-tidy, not sure what I'm doing...)

Jul 13 2018, 3:20 AM
sammccall added a reviewer for D49285: [clang-tidy] readability-inconsistent-declaration-parameter-name: accept approximate name matches.: hokein.
Jul 13 2018, 3:20 AM
sammccall updated the diff for D49285: [clang-tidy] readability-inconsistent-declaration-parameter-name: accept approximate name matches..

Make strictness (old behavior) available as an option.

Jul 13 2018, 3:19 AM
sammccall created D49285: [clang-tidy] readability-inconsistent-declaration-parameter-name: accept approximate name matches..
Jul 13 2018, 2:58 AM

Jul 12 2018

sammccall committed rCTE336909: [clangd] Extract FileSystemProvider into a separate header. NFC.
[clangd] Extract FileSystemProvider into a separate header. NFC
Jul 12 2018, 7:55 AM
sammccall committed rL336909: [clangd] Extract FileSystemProvider into a separate header. NFC.
[clangd] Extract FileSystemProvider into a separate header. NFC
Jul 12 2018, 7:55 AM
sammccall closed D49142: [clangd] Extract FileSystemProvider into a separate header. NFC.
Jul 12 2018, 7:54 AM
sammccall accepted D49142: [clangd] Extract FileSystemProvider into a separate header. NFC.
Jul 12 2018, 7:45 AM
sammccall committed rCTE336899: [clangd] log request/response messages with method/ID/error at INFO level.
[clangd] log request/response messages with method/ID/error at INFO level
Jul 12 2018, 4:57 AM
sammccall committed rL336899: [clangd] log request/response messages with method/ID/error at INFO level.
[clangd] log request/response messages with method/ID/error at INFO level
Jul 12 2018, 4:57 AM
sammccall closed D49224: [clangd] log request/response messages with method/ID/error at INFO level.
Jul 12 2018, 4:57 AM
sammccall created D49224: [clangd] log request/response messages with method/ID/error at INFO level.
Jul 12 2018, 2:14 AM
sammccall committed rCTE336890: [clangd] Simplify logging wrapper after r336888.
[clangd] Simplify logging wrapper after r336888
Jul 12 2018, 1:06 AM
sammccall committed rL336890: [clangd] Simplify logging wrapper after r336888.
[clangd] Simplify logging wrapper after r336888
Jul 12 2018, 1:05 AM
sammccall committed rL336888: [Support] Require llvm::Error passed to formatv() to be wrapped in fmt_consume().
[Support] Require llvm::Error passed to formatv() to be wrapped in fmt_consume()
Jul 12 2018, 12:16 AM
sammccall closed D49170: [Support] Require llvm::Error passed to formatv() to be wrapped in fmt_consume().
Jul 12 2018, 12:16 AM
sammccall abandoned D49013: [Support] Allow formatv() to consume llvm::Error by-value without crashing..

Would it be reasonable to have the wrapper for an Error value call consumeError in its destructor? That way a filtered value would silently consume the error. A printed value would consume it twice, but that should be safe (if it isn't already then we can make it safe).

Jul 12 2018, 12:15 AM

Jul 11 2018

sammccall accepted D49175: [clangd] Ignore sema code complete callback with recovery context..

I like this idea, of course hard to know how it will affect all practical cases.

Jul 11 2018, 5:48 AM
sammccall committed rCTE336785: [clangd] Upgrade logging facilities with levels and formatv..
[clangd] Upgrade logging facilities with levels and formatv.
Jul 11 2018, 3:40 AM
sammccall committed rL336785: [clangd] Upgrade logging facilities with levels and formatv..
[clangd] Upgrade logging facilities with levels and formatv.
Jul 11 2018, 3:40 AM
sammccall closed D49008: [clangd] Upgrade logging facilities with levels and formatv..
Jul 11 2018, 3:40 AM
sammccall updated the diff for D49008: [clangd] Upgrade logging facilities with levels and formatv..

Add workaround for formatv+error issues.

Jul 11 2018, 3:38 AM
sammccall removed a dependency for D49008: [clangd] Upgrade logging facilities with levels and formatv.: D49013: [Support] Allow formatv() to consume llvm::Error by-value without crashing..
Jul 11 2018, 3:37 AM
sammccall removed a dependent revision for D49013: [Support] Allow formatv() to consume llvm::Error by-value without crashing.: D49008: [clangd] Upgrade logging facilities with levels and formatv..
Jul 11 2018, 3:37 AM
sammccall updated the diff for D49170: [Support] Require llvm::Error passed to formatv() to be wrapped in fmt_consume().

Missing inline

Jul 11 2018, 2:09 AM
sammccall created D49170: [Support] Require llvm::Error passed to formatv() to be wrapped in fmt_consume().
Jul 11 2018, 1:20 AM
sammccall abandoned D49129: [Support] raw_ostream << Error&& and formatv(..., Error&&) consume the Error..

As pointed out on D49013, this doesn't work for formatv, as the behavior is different when printing 0/1/2 times.

Jul 11 2018, 12:24 AM
sammccall closed D46274: [Support] Harden JSON against invalid UTF-8..

Landed as r336657, not sure why it didn't get linked to this review.

Jul 11 2018, 12:22 AM

Jul 10 2018

sammccall added a comment to D49013: [Support] Allow formatv() to consume llvm::Error by-value without crashing..

In the samples you gave in the original code review, it was always just
formatv(“Error: {0}”, E);

Is it always this simple and consistent? Or do you frequently construct
complex messages with many other parameters in the same format string? If
it’s always (or even often) that simple, maybe you just need a function
like void logError(Error E)

Jul 10 2018, 8:57 AM
sammccall updated the summary of D49129: [Support] raw_ostream << Error&& and formatv(..., Error&&) consume the Error..
Jul 10 2018, 7:06 AM
sammccall added a comment to D49013: [Support] Allow formatv() to consume llvm::Error by-value without crashing..

An alternate approach in D49129: give Error an overloaded operator<< that has the same consume semantics at toString when passed rvalues.

Jul 10 2018, 7:03 AM
sammccall updated the diff for D49129: [Support] raw_ostream << Error&& and formatv(..., Error&&) consume the Error..

Remove some noise

Jul 10 2018, 6:57 AM
sammccall created D49129: [Support] raw_ostream << Error&& and formatv(..., Error&&) consume the Error..
Jul 10 2018, 6:56 AM
sammccall committed rL336657: [Support] Harded JSON against invalid UTF-8..
[Support] Harded JSON against invalid UTF-8.
Jul 10 2018, 4:56 AM
sammccall updated the diff for D46274: [Support] Harden JSON against invalid UTF-8..

Make LLVM_[UN]LIKELY top-level

Jul 10 2018, 4:54 AM
sammccall added inline comments to D46274: [Support] Harden JSON against invalid UTF-8..
Jul 10 2018, 4:53 AM
sammccall added a comment to D49013: [Support] Allow formatv() to consume llvm::Error by-value without crashing..

Thanks for looking at this, and sorry it's a complicated space.
Context is: I'm trying to move clangd's logging to wrap formatv instead of ad-hoc concatenation. One of the motivations is to make dealing with llvm::Error/Expected less painful. (In hindsight, we maybe should have rolled our own type here maybe, but Error is pervasive in our code now).

Jul 10 2018, 1:21 AM

Jul 9 2018

sammccall added inline comments to D46274: [Support] Harden JSON against invalid UTF-8..
Jul 9 2018, 8:53 AM
sammccall updated the diff for D46274: [Support] Harden JSON against invalid UTF-8..

Properly rebase this time, and add/use isASCII in StringExtras.

Jul 9 2018, 8:53 AM
sammccall committed rL336549: [clangd] Remove JSON library in favor of llvm/Support/JSON.
[clangd] Remove JSON library in favor of llvm/Support/JSON
Jul 9 2018, 7:31 AM
sammccall committed rCTE336549: [clangd] Remove JSON library in favor of llvm/Support/JSON.
[clangd] Remove JSON library in favor of llvm/Support/JSON
Jul 9 2018, 7:31 AM
sammccall closed D49077: [clangd] Remove JSON library in favor of llvm/Support/JSON.
Jul 9 2018, 7:30 AM
sammccall accepted D49028: [clangd] Support indexing MACROs..
Jul 9 2018, 7:20 AM
sammccall created D49077: [clangd] Remove JSON library in favor of llvm/Support/JSON.
Jul 9 2018, 7:09 AM
sammccall updated the diff for D46274: [Support] Harden JSON against invalid UTF-8..

Rebase and format

Jul 9 2018, 6:04 AM