• Rebase
• Update some comments

LGTM

LGTM

• Address comments, document code.
• s/Expansion/CollectedExpansions.
• Added FIXMEs for macro arguments.

Thanks, this looks very good!
Leaving a few nitpicky comments, but nothing important really.

Sorry about the confusion, I can now see why the original version of the patch was actually simpler.
I was put off by the fact that we override by adding command-line arguments instead of passing things around in the code, but it appears that's actually simpler than adding yet another mechanism to override target tripple in the driver.

After looking at this, adjusting arguments on the clangd side seems to be a better approach.
Let's abandon this.

Although there are still rough edges, I believe the storage model is agreed upon and we can hopefully address the rest in the follow-ups.

This is ready for another round now.

• A few more renames and docs
• Cleanups and comments
• Reformat the code

LGTM.

• Do the renames

To clarify, I don't think there are new concepts in this patch.
Previously, we only took information from source locations into account when building token buffers. That works fine in most cases, but not enough to find the boundaries of empty macro expansions.
In order to find those, we now also watch the callbacks from the preprocessor to get all macro expansions (incuding the empty ones) and reconstruct the mappings of token buffers from there.

• Address comments

In D62956#1547826, @sammccall wrote:

Can we add a test using TestTU that does a very basic verification of expanded/spelled tokens (first after preamble, last token in file)?

• Rename tokens() to getTokens() for consistency with the rest of ParsedAST.
• Update comments.
• Add a test.

In D62953#1547799, @sammccall wrote:

Can you explain why this is important?
(in the code)

• Added comments.

• Added a message to the assertion

• Address comments.
• s/findExpansion/expansionStartingAt.
• Add tests.

LGTM with a few NITs

Nice, thanks! The clangd parts obviously LGTM, but let's be more conservative with the driver bits (see the relevant comment)

This is not 100% ready yet, but wanted to send it out anyway, as I'll be on vacation until Tuesday.

• A leaf node stores a single token
• Restructure code to avoid special-casing leaf nodes

@hokein just realized you might be a better reviewer, since this makes applyTweak aligned with rename. And you implemented rename in the first place

LGTM

LGTM

Notes from the offline discussion:

• Sam: supporting multiple typed text chunks is hard, many clients that expect only one. Objective-C already does that, though, but in a way that does not break most targets.
• Sam: supporting any other form of custom text for filtering is also hard, particularly in clangd that exposes a structured completion item with qualifiers, etc.
• me: feel strongly that we should show case at the start and match it.
• me: also feel strongly that the feature is useful. Having a single big "typed text" chunk that includes the qualifier and 'case' looks better than not having the feature.

In D62804#1538155, @kadircet wrote:

For example a gcc cross compiling to arm comes with its own system includes and has some mechanisms to discover that implicitly, without requiring any "-I" flags.
Hence when used with clangd, we make use of system includes instead of the target's include library. This patch aims to solve that issue, tools like cquery also handles that problem in a similar way.

NIT:
I've been pushing towards giving clangd-specific extensions names that would absolutely never clash with anything that LSP proposes. In particular, the file status notification is textDocument/clangd.fileStatus.
With that in line, I propose to use clangd.editsNearCursor for the flag name. Other than that, LGTM.

Not displaying 'case' is actually confusing to my taste.
WDYT about allowing multiple "typed text" chunks or allowing to mark other chunks that are supposed be part of filter text?

• Return empty results instead of an error

Could you give more context on what the custom toolchains are?
One feasible alternative is to move this detection to clang's driver (where the rest of include path detection lives), why won't that work?

LGTM, I see no way around it, we can't copy all of the shared libs.
Maybe there's an alternative way to test this without copying the binary, but I don't have one in mind.

• Move logically separate parts to other changes

In D62621#1529691, @ymandel wrote:

Thanks for the review.

Good questions. The answer to both is that it depends on the RangeSelector used. change itself takes exactly the range given it and modifies the source at that range. Since these APIs are acting at the character level, I think it could be confusing to add token-level reasoning. That said, we could add RangeSelectors and/or TextGenerators that are smarter about this and ensure/add spaces around text as needed. Since all changes should (in my opinion) be run through clang-format afterwards, there's no risk in adding extra whitespace. For example, we could change text() to add whitespace on both sides of the argument, or add a new combinator pad which does that (and leave text() alone). So, for this example, the user would write insertBefore([[FOO]], pad("BAR")) and get BAR FOO.

We could similarly update Stencil to pad its output by default or somesuch.

WDYT?

@jkorous, could you take a look or suggest someone who can review the libclang changes?
As mentioned before, I've reviewed the clang bits (which are now gone), but don't have much experience in libclang parts.

LGTM

Please add a check for the type of variable being not null before landing. The other comment is not very important

In D62814#1528736, @kadircet wrote:

Nice catch! I think it makes sense to show signature in those cases as well.
Updating according to that.

I guess the question also applies to change, although it did not occur to me before.

Are there any corner cases with handling whitespace that we might want to handle or document them if we don't handle them?
E.g. if the range ends up being a name of an expanded macro , it's probably very easy to glue the macro name with whatever you're inserting.

Do we care about pointers or references to lambda types?

I've addressed most of the comments, except the naming ones.
We need a convention for naming the language nodes and names for composite and leaf structural nodes.

• Address comments

LGTM

In D62616#1522284, @gribozavr wrote:

I totally agree about the space before the opening curly, but the space before the opening paren is a matter of style. Certainly it does match LLVM style better, but it does not match some other styles.

I guess there's no way to satisfy everyone without an automatic reformatter, and if we were to pick a "default" style it might as well be LLVM, so I approve.