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 (156 w, 9 h)
Roles
Administrator

Recent Activity

Today

sammccall updated subscribers of rL369349: [clangd] Skip function bodies inside processed files while indexing.

I think we concluded we wanted to merge this into the release, which is apparently still possible. @hans

Fri, Aug 23, 8:19 AM
sammccall accepted D66650: clang-format: chromium style: Disable across-block include reordering..

How's the work going to undo r357695 btw?

Fri, Aug 23, 6:52 AM · Restricted Project
sammccall added inline comments to D66637: [clangd] Support multifile edits as output of Tweaks.
Fri, Aug 23, 5:48 AM · Restricted Project
sammccall added inline comments to D66637: [clangd] Support multifile edits as output of Tweaks.
Fri, Aug 23, 5:38 AM · Restricted Project
sammccall accepted D66632: [clangd] Link more clang-tidy modules to clangd.
Fri, Aug 23, 1:49 AM · Restricted Project, Restricted Project

Yesterday

sammccall added inline comments to D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include.
Thu, Aug 22, 7:06 AM · Restricted Project
sammccall updated the diff for D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include.

kick phabricator to try to get it to mail

Thu, Aug 22, 7:01 AM · Restricted Project
sammccall created D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include.
Thu, Aug 22, 7:00 AM · Restricted Project

Wed, Aug 21

sammccall committed rGa451156bb6ce: reland [gtest] Fix printing of StringRef and SmallString in assert messages. (authored by sammccall).
reland [gtest] Fix printing of StringRef and SmallString in assert messages.
Wed, Aug 21, 6:57 AM
sammccall committed rL369527: reland [gtest] Fix printing of StringRef and SmallString in assert messages..
reland [gtest] Fix printing of StringRef and SmallString in assert messages.
Wed, Aug 21, 6:55 AM
sammccall committed rGe7c0356b69ac: Revert "[gtest] Fix printing of StringRef and SmallString in assert messages." (authored by sammccall).
Revert "[gtest] Fix printing of StringRef and SmallString in assert messages."
Wed, Aug 21, 6:34 AM
sammccall committed rL369525: Revert "[gtest] Fix printing of StringRef and SmallString in assert messages.".
Revert "[gtest] Fix printing of StringRef and SmallString in assert messages."
Wed, Aug 21, 6:31 AM
sammccall committed rG2fe9ce606407: [gtest] Fix printing of StringRef and SmallString in assert messages. (authored by sammccall).
[gtest] Fix printing of StringRef and SmallString in assert messages.
Wed, Aug 21, 4:37 AM
sammccall committed rL369518: [gtest] Fix printing of StringRef and SmallString in assert messages..
[gtest] Fix printing of StringRef and SmallString in assert messages.
Wed, Aug 21, 4:37 AM
sammccall closed D66520: [gtest] Fix printing of StringRef and SmallString in assert messages..
Wed, Aug 21, 4:37 AM · Restricted Project
sammccall added inline comments to D66520: [gtest] Fix printing of StringRef and SmallString in assert messages..
Wed, Aug 21, 4:34 AM · Restricted Project
sammccall updated the diff for D66520: [gtest] Fix printing of StringRef and SmallString in assert messages..

review comments

Wed, Aug 21, 4:34 AM · Restricted Project
sammccall updated the diff for D66520: [gtest] Fix printing of StringRef and SmallString in assert messages..

revert extraneous include

Wed, Aug 21, 3:04 AM · Restricted Project
sammccall added a comment to D65633: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 3].

I've been using ::testing::ElementsAre(...) against a vector of StringRef a lot, when it fails the output is really bad. Like this: element #1 is equal to { '.' (46, 0x2E), 's' (115, 0x73), 'e' (101, 0x65), 'c' (99, 0x63), '0' (48, 0x30) }, Is there a way to provide formatting for types to gtest? Or should I just be using const char *?

Yeah, there is. It involves writing either an operator<< or a PrintTo function. The StringRef class already has an operator<<, but the problem here is that gtest wants to print it as a container instead of using that operator<<. I don't know if there's anything we can do here, but @sammccall might.

This has bothered at various times, but I never really investigated how to fix it. I'll try to put together a patch.

Wed, Aug 21, 2:58 AM · Restricted Project
sammccall created D66520: [gtest] Fix printing of StringRef and SmallString in assert messages..
Wed, Aug 21, 2:57 AM · Restricted Project
sammccall added a comment to D65633: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 3].

I've been using ::testing::ElementsAre(...) against a vector of StringRef a lot, when it fails the output is really bad. Like this: element #1 is equal to { '.' (46, 0x2E), 's' (115, 0x73), 'e' (101, 0x65), 'c' (99, 0x63), '0' (48, 0x30) }, Is there a way to provide formatting for types to gtest? Or should I just be using const char *?

Yeah, there is. It involves writing either an operator<< or a PrintTo function. The StringRef class already has an operator<<, but the problem here is that gtest wants to print it as a container instead of using that operator<<. I don't know if there's anything we can do here, but @sammccall might.

Right, the sequence is:

  • PrintTo() is found by overload resolution, if there's no specialized version, it falls back to the generic template
  • the generic template checks whether it's a container (by looking for nested iterator and const_iterator types)
  • for non-containers, it attempts to use operator<< if it exists, falling back to "n-byte object XX-XX-XX-XX" otherwise
  • for containers, it prints each element in the same manner

So overloading operator<< doesn't bypass the container detection, but overloading PrintTo() does. And this is what gtest does for std::string, in gtest-printers.h. I think we should do the same as a local modification to internal/custom/gtest-printers.h.

Wed, Aug 21, 1:31 AM · Restricted Project

Tue, Aug 20

sammccall added a comment to D65526: [Clangd] First version of ExtractFunction.

Main themes are:

  • there are a few places we can reduce scope for the first patch: e.g. template support
  • internally, it'd be clearer to pass data around in structs and avoid complex classes unless the abstraction is important
  • high-level documentation would aid in understanding: for each concept/function: what is the goal, how does it fit into higher-level goals, are there any non-obvious reasons it's done this way
  • naming questions (this can solve some of the same problems as docs, but much more cheaply)
Tue, Aug 20, 4:11 AM · Restricted Project

Thu, Aug 15

sammccall added a comment to rL369005: [clangd] Don't use Bind() where C++14 move capture works.

This is glorious, thank you!

Thu, Aug 15, 11:40 AM

Wed, Aug 14

sammccall accepted D66212: Removed ToolExecutor::isSingleProcess, it is not used by anything.
Wed, Aug 14, 4:25 AM · Restricted Project, Restricted Project
sammccall added a comment to D66172: [clang][Modules] Serialize decl to comment mapping to speed up code completion..

+ilya because I know he's wrestled with these before, and because I'm OOO sick

Wed, Aug 14, 1:09 AM
sammccall added a reviewer for D66172: [clang][Modules] Serialize decl to comment mapping to speed up code completion.: ilya-biryukov.
Wed, Aug 14, 1:08 AM

Mon, Aug 12

sammccall committed rG6a3c2c84be2b: [clangd] Refactor computation of extracted expr in ExtractVariable tweak. NFC (authored by sammccall).
[clangd] Refactor computation of extracted expr in ExtractVariable tweak. NFC
Mon, Aug 12, 10:07 AM
sammccall committed rL368590: [clangd] Refactor computation of extracted expr in ExtractVariable tweak. NFC.
[clangd] Refactor computation of extracted expr in ExtractVariable tweak. NFC
Mon, Aug 12, 10:07 AM
sammccall closed D65333: [clangd] Refactor computation of extracted expr in ExtractVariable tweak. NFC.
Mon, Aug 12, 10:07 AM · Restricted Project
sammccall updated the diff for D65333: [clangd] Refactor computation of extracted expr in ExtractVariable tweak. NFC.

rebase

Mon, Aug 12, 10:07 AM · Restricted Project
sammccall closed D64541: rL365634 adds a unique_ptr<CompilationDatabase> in GobalCompilationDatabase.h:108 but CompilationDatabase is only forward declared. This makes the header not compile standalone, because unique_ptrs expect to have the full-definition of the....
Mon, Aug 12, 7:43 AM · Restricted Project
sammccall added a comment to D66087: [clangd] Preserve line break when rendering text chunks of markdown.

Added a bit of a braindump to https://github.com/clangd/clangd/issues/95

Mon, Aug 12, 7:38 AM · Restricted Project
sammccall accepted D66086: [clangd] Separate chunks with a space when rendering markdown.
Mon, Aug 12, 6:45 AM · Restricted Project, Restricted Project
sammccall accepted D65643: raw_ostream: add operator<< overload for std::error_code.

Thanks! I actually like the ASSERT_NO_ERROR macros, std::error_code() meaning success is pretty obscure to write everywhere.

Mon, Aug 12, 6:00 AM · Restricted Project
sammccall accepted D65373: [clangd] Update features table in the docs with links to LSP extension proposals.

Sorry, I had marked this as "accepted", but not hit send. I agree with Ilya's comments, but please go ahead and land this when ready.

Mon, Aug 12, 5:39 AM · Restricted Project, Restricted Project
sammccall accepted D65752: [Sema] Refactor LookupVisibleDecls. NFC.

Agreed, this seems fine as is.

Mon, Aug 12, 4:50 AM · Restricted Project

Fri, Aug 9

sammccall committed rG1aaef90c2aab: [clangd] Disallow extraction of expression-statements. (authored by sammccall).
[clangd] Disallow extraction of expression-statements.
Fri, Aug 9, 4:43 PM
sammccall committed rL368500: [clangd] Disallow extraction of expression-statements..
[clangd] Disallow extraction of expression-statements.
Fri, Aug 9, 4:43 PM
sammccall closed D65337: [clangd] Disallow extraction of expression-statements..
Fri, Aug 9, 4:43 PM · Restricted Project, Restricted Project
sammccall updated subscribers of D66031: clangd: use -j for background index pool.

@hans If you don't mind merging this, it's a nice usability improvement.

Fri, Aug 9, 4:42 PM · Restricted Project, Restricted Project
sammccall added a comment to D65044: [Format] Add option to enable coroutines keywords.

Do we have real examples of code that gets formatted incorrectly when parsed as c++20 but lives in a codebase that uses coroutines TS?
This seems very niche (and probably short-lived) - do we really need this option?

Fri, Aug 9, 4:37 PM · Restricted Project
sammccall added a comment to D65752: [Sema] Refactor LookupVisibleDecls. NFC.

This looks reasonable to me, there are a couple of variations you might think about:

  • also treat QualifiedNameLookup as an option, and override in places with an RAII pattern like ScopedOverride Unqual(QualifiedNameLookup, false) (why don't we have a class like that?). This would further reduce args.
  • put the options in a LookupOpts struct and pass it by const reference - this is a less invasive change
Fri, Aug 9, 4:24 PM · Restricted Project
sammccall committed rG4bd6ebb49584: clangd: use -j for background index pool (authored by sammccall).
clangd: use -j for background index pool
Fri, Aug 9, 4:07 PM
sammccall committed rL368498: clangd: use -j for background index pool.
clangd: use -j for background index pool
Fri, Aug 9, 4:07 PM
sammccall closed D66031: clangd: use -j for background index pool.
Fri, Aug 9, 4:06 PM · Restricted Project, Restricted Project
sammccall retitled D66038: [Support] heavyweight_hardware_concurrency uses affinity when counting cores fails, and never returns 0 from [clangd] Give absolute path to clang-tidy and include-fixer. HintPath should always be absolute, some URI schemes care. to [Support] heavyweight_hardware_concurrency uses affinity when counting cores fails, and never returns 0.
Fri, Aug 9, 4:06 PM · Restricted Project, Restricted Project
sammccall updated the diff for D66038: [Support] heavyweight_hardware_concurrency uses affinity when counting cores fails, and never returns 0.

Rebase

Fri, Aug 9, 4:06 PM · Restricted Project, Restricted Project
sammccall planned changes to D66038: [Support] heavyweight_hardware_concurrency uses affinity when counting cores fails, and never returns 0.

ugh, I accidentally reused a branch and squashed two patches together. Let me try again...

Fri, Aug 9, 4:06 PM · Restricted Project, Restricted Project
sammccall created D66038: [Support] heavyweight_hardware_concurrency uses affinity when counting cores fails, and never returns 0.
Fri, Aug 9, 3:57 PM · Restricted Project, Restricted Project
sammccall accepted D66031: clangd: use -j for background index pool.

Thanks! Want me to land this for you?

Fri, Aug 9, 3:45 PM · Restricted Project, Restricted Project
sammccall added a comment to D65043: [Format] Add C++20 standard to style options.

Sorry for taking so long here, I've been swamped.

Fri, Aug 9, 3:40 PM · Restricted Project
sammccall accepted D65925: [clang-format] Add SpaceInEmptyBlock option for WebKit.
Fri, Aug 9, 3:13 PM · Restricted Project, Restricted Project
sammccall added inline comments to D66031: clangd: use -j for background index pool.
Fri, Aug 9, 2:59 PM · Restricted Project, Restricted Project
sammccall added a comment to D65643: raw_ostream: add operator<< overload for std::error_code.

@sammccall, what do you think of the operator<<(raw_ostream&, std::error_code) idea? Is that the direction I should go in? If so, where would the implementation of that function live? (next to raw_ostream, somewhere under llvm/Testing, or under gtest/internal/custom)

Fri, Aug 9, 2:45 PM · Restricted Project
sammccall accepted D66031: clangd: use -j for background index pool.

Yeah, this is much better than only having -background-index=no.

Fri, Aug 9, 2:18 PM · Restricted Project, Restricted Project
sammccall committed rG06431b2b047a: [clangd] Give absolute path to clang-tidy and include-fixer. HintPath should… (authored by sammccall).
[clangd] Give absolute path to clang-tidy and include-fixer. HintPath should…
Fri, Aug 9, 1:46 PM
sammccall committed rL368482: [clangd] Give absolute path to clang-tidy and include-fixer. HintPath should….
[clangd] Give absolute path to clang-tidy and include-fixer. HintPath should…
Fri, Aug 9, 1:46 PM
sammccall committed rG0c1da4a79690: Rename PCH/leakfiles test so it runs on bots. (authored by sammccall).
Rename PCH/leakfiles test so it runs on bots.
Fri, Aug 9, 10:15 AM
sammccall added a comment to D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak..

Renamed in rL368455

Fri, Aug 9, 10:13 AM · Restricted Project
sammccall committed rL368455: Rename PCH/leakfiles test so it runs on bots..
Rename PCH/leakfiles test so it runs on bots.
Fri, Aug 9, 10:13 AM
sammccall accepted D65936: [clangd] Use raw rename functions to implement the rename..
Fri, Aug 9, 1:47 AM · Restricted Project, Restricted Project
sammccall updated subscribers of rL368058: Fixed toHalfOpenFileRange assertion fail.

@hans is there still time to merge this?

Fri, Aug 9, 12:54 AM
sammccall added a comment to D56545: [VFS] Allow multiple RealFileSystem instances with independent CWDs..

Thanks for the heads up!
I think the breakage was rather from D62271 (when this was written, there was no plan to switch the default)

Fri, Aug 9, 12:41 AM · Restricted Project
sammccall updated subscribers of D65986: Allow setting the VFS to 'real' mode instead of default 'physical'.

Tagging D62271 and @Bigcheese as this was the change that switched the default in Driver. (My motivation for D58169 was thread-heavy programs like clangd).

Fri, Aug 9, 12:05 AM · Restricted Project

Thu, Aug 8

sammccall added inline comments to D65936: [clangd] Use raw rename functions to implement the rename..
Thu, Aug 8, 5:49 AM · Restricted Project, Restricted Project
sammccall added inline comments to D65936: [clangd] Use raw rename functions to implement the rename..
Thu, Aug 8, 4:00 AM · Restricted Project, Restricted Project
sammccall accepted D65883: [Extract] Fixed SemicolonExtractionPolicy for SwitchStmt and SwitchCase.
Thu, Aug 8, 1:31 AM · Restricted Project, Restricted Project

Wed, Aug 7

sammccall committed rG0e8dd4a80e74: Code completion should not ignore default parameters in functions. (authored by sammccall).
Code completion should not ignore default parameters in functions.
Wed, Aug 7, 9:53 AM
sammccall committed rL368186: Code completion should not ignore default parameters in functions..
Code completion should not ignore default parameters in functions.
Wed, Aug 7, 9:51 AM
sammccall closed D65866: Code completion should not ignore default parameters in functions..
Wed, Aug 7, 9:51 AM · Restricted Project, Restricted Project
sammccall accepted D65866: Code completion should not ignore default parameters in functions..
Wed, Aug 7, 7:46 AM · Restricted Project, Restricted Project
sammccall accepted D65833: [Tooling] Expose ExecutorConcurrency option..

Only hesitation is that AllTUsExecution is one executor of (potentially) many, so this is a weird place for a common flag.

Wed, Aug 7, 12:53 AM · Restricted Project

Tue, Aug 6

sammccall committed rG957380714da3: [clangd] Unfold SourceLocation flattening from findNameLoc in preparation for… (authored by sammccall).
[clangd] Unfold SourceLocation flattening from findNameLoc in preparation for…
Tue, Aug 6, 1:29 PM
sammccall committed rL368083: [clangd] Unfold SourceLocation flattening from findNameLoc in preparation for….
[clangd] Unfold SourceLocation flattening from findNameLoc in preparation for…
Tue, Aug 6, 1:25 PM
sammccall accepted D65754: Fix toHalfOpenFileRange assertion fail.

Sorry, should have accepted this in the last round. Great stuff, thank you!

Tue, Aug 6, 9:42 AM · Restricted Project, Restricted Project
sammccall added inline comments to D65754: Fix toHalfOpenFileRange assertion fail.
Tue, Aug 6, 7:55 AM · Restricted Project, Restricted Project
sammccall added inline comments to D65754: Fix toHalfOpenFileRange assertion fail.
Tue, Aug 6, 6:31 AM · Restricted Project, Restricted Project
sammccall added a comment to D65677: [VirtualFileSystem] Support encoding a current working directory in a VFS mapping..

I'd like to know if you tried other approaches and why they failed.

Tue, Aug 6, 3:39 AM · Restricted Project, Restricted Project
sammccall accepted D65796: [clangd] Compute scopes eagerly in IncludeFixer.
Tue, Aug 6, 3:34 AM · Restricted Project, Restricted Project

Mon, Aug 5

sammccall accepted D65387: [clangd] Add a callback mechanism for handling responses from client..
Mon, Aug 5, 5:44 AM · Restricted Project, Restricted Project
sammccall requested changes to D65677: [VirtualFileSystem] Support encoding a current working directory in a VFS mapping..

Oops, didn't mean to mark as accepted.

Mon, Aug 5, 5:34 AM · Restricted Project, Restricted Project
sammccall accepted D65677: [VirtualFileSystem] Support encoding a current working directory in a VFS mapping..

It seems conceptually a little strange to have the working directory be part of a serialized "FS", as it's fundamentally a property of a process and only transiently a property of the VFS.
I don't know this is fatal (not sure what else the RedirectingFileSystem might be used for.

Mon, Aug 5, 5:33 AM · Restricted Project, Restricted Project
sammccall accepted D65387: [clangd] Add a callback mechanism for handling responses from client..
Mon, Aug 5, 5:19 AM · Restricted Project, Restricted Project
sammccall accepted D63835: [Syntax] Add nodes for most common statements.
Mon, Aug 5, 4:20 AM · Restricted Project
sammccall accepted D64576: [Syntax] Do not add a node for 'eof' into the tree.
Mon, Aug 5, 1:37 AM · Restricted Project, Restricted Project
sammccall committed rG801d3304e9ed: [clangd] Expose -offset-encoding=utf-32, which has been implemented for ages (authored by sammccall).
[clangd] Expose -offset-encoding=utf-32, which has been implemented for ages
Mon, Aug 5, 1:15 AM
sammccall committed rG6b09e9c86484: [clangd] Fix error message with incorrect TextDocumentcontentChangeEvent. (authored by sammccall).
[clangd] Fix error message with incorrect TextDocumentcontentChangeEvent.
Mon, Aug 5, 1:14 AM
sammccall committed rL367812: [clangd] Expose -offset-encoding=utf-32, which has been implemented for ages.
[clangd] Expose -offset-encoding=utf-32, which has been implemented for ages
Mon, Aug 5, 1:14 AM
sammccall committed rL367811: [clangd] Fix error message with incorrect TextDocumentcontentChangeEvent..
[clangd] Fix error message with incorrect TextDocumentcontentChangeEvent.
Mon, Aug 5, 1:14 AM

Fri, Aug 2

sammccall added a comment to rL367616: [clang] Adopt new FileManager error-returning APIs.

This appears to have missed lib/Index/SimpleFormatContext.h.
AFAICT it's unused and untested, but it used to parse and now it doesn't. It should probably either be updated or removed from the tree.

Fri, Aug 2, 2:44 PM
sammccall added a comment to D65387: [clangd] Add a callback mechanism for handling responses from client..

Thanks, this looks a lot better. There are a bunch of details that can be simplified and names that could be clearer, but I think this is the right shape.

Fri, Aug 2, 2:29 PM · Restricted Project, Restricted Project
sammccall accepted D65657: [clangd][vscode] clang-format the the extension code..
Fri, Aug 2, 7:08 AM · Restricted Project, Restricted Project
sammccall added inline comments to D65526: [Clangd] First version of ExtractFunction.
Fri, Aug 2, 7:06 AM · Restricted Project
sammccall accepted D65655: [clangd] Fix a crash when presenting values for Hover.
Fri, Aug 2, 6:47 AM · Restricted Project, Restricted Project
sammccall added a comment to D65526: [Clangd] First version of ExtractFunction.

I guess you're looking for design review at this point.

Fri, Aug 2, 5:54 AM · Restricted Project
sammccall committed rGad66e95b0de5: [clangd] Remove bad assert: nothing relies on it, and the reasons it was true… (authored by sammccall).
[clangd] Remove bad assert: nothing relies on it, and the reasons it was true…
Fri, Aug 2, 3:42 AM
sammccall committed rL367672: [clangd] Remove bad assert: nothing relies on it, and the reasons it was true….
[clangd] Remove bad assert: nothing relies on it, and the reasons it was true…
Fri, Aug 2, 3:42 AM
sammccall added a comment to D65643: raw_ostream: add operator<< overload for std::error_code.

I'm lacking a bit of context here.
Why do we prefer this to EXPECT_FALSE(err) /EXPECT_EQ(err, std::errc_address_in_use) with an appropriate value printer for std::error_code?

Fri, Aug 2, 3:24 AM · Restricted Project
sammccall committed rGac7864ec019c: [clangd] Add new helpers to make tweak tests scale better. Convert most tests. (authored by sammccall).
[clangd] Add new helpers to make tweak tests scale better. Convert most tests.
Fri, Aug 2, 2:13 AM
sammccall committed rL367667: [clangd] Add new helpers to make tweak tests scale better. Convert most tests..
[clangd] Add new helpers to make tweak tests scale better. Convert most tests.
Fri, Aug 2, 2:13 AM