Sure, happy to add some basic tests.
Unfortunately did not get to this today and I'm on vacation until next Tuesday. Will get back to this after I'm back.

• Simplify rawByExpanded by using a helper function.

Not sure if you want to land this before or after stencil. This seems useful even without the latter.

LGTM!

In D60510#1467262, @dblaikie wrote:

Might've made sense to separate the NFC renaming from the perfect forwarding change (& the perfect forwarding change could potentially have some matching test coverage?)

I agree, sorry about mashing them in together. That said, I was confident this wouldn't cause any trouble in practice.
What kinds of tests are you thinking about? Checking that llvm::lower_bound works on non-copyable types?

Thanks for the update, looks good! Just a few nits from my side.

Sorry, lost this revision. Looking now

A few quick responses, will take a closer look again tomorrow.

LGTM! See the NITs, specifically about runAddDocument - those definitely look worth fixing.

Thanks, the change LG now. Only nits from my side!

In D60408#1464364, @ymandel wrote:

I've done that as far as I can tell. Please let me know if I've missed anything.

Many thanks!

This is long overdue

Yay! LGTM

LGTM

Sorry for the delay.
There seem to be a few changes that are unrelated to the actual patch. Could we separate various non-functional changes (moving code around, etc.) into a separate change to keep the diff for this one minimal?

To make it clear, I think the question is not just "which part of functionality is missing in BackgroundIndex", it's rather "which part of BackgroundIndex we don't need".

To reiterate the offline discussion: the tool seems useful to me, but it would be best to keep the client code simpler, it's currently fighting with BackgroundIndex because it's trying to hijack some of its functionality.
More specifically, I propose to add a function to BackgroundIndex.h that would be a one-shot update of the index, the client code would become significantly simpler and we would have more flexibility in how we move code around in BackgroundIndex.cpp to actually make this happen.

• Store a source manager in a token buffer

The new version address most of the comments, there are just a few left in the code doing the mapping from Dmitri, I'll look into simplifying and removing the possibly redundant checks.
Please take a look at this version, this is very close to the final state.

• Fix header comments.

• Remove a spamy debug tracing output.
• Less debug output, move stuff around, more comments.
• Add methods that map expanded tokens to raw tokens.
• Rename toOffsetsRange.
• Remove default ctor of Token.
• Introduce a struct for storing FileID and a pair of offsets.
• Update comments.
• Add a test for macro expansion at the end of the file.
• Misc fixes.

LG, just have a simple clarifying question.

• clang-format

I would propose to change the name of the tool.
The "background" part only makes sense in the context of running inside clangd, the index stored on disk is not "background" in any manner.
I'd go with something like build-clangd-index (the best option is probably clangd-indexer, but it's is already taken and we don't want to confuse the two tools).

Changes:

• Add multi-file support, record a single expanded stream and per-file-id raw token streams and mappings.
• Rename MacroInvocation to TokenBuffer::Mapping, make it private.
• Simplify TokenCollector, let preprocessor handle some more stuff.

Pass an enum indicating where the token comes from.
The enum is ad-hoc at the moment, will require some thought to turn
it into a reasonable abstraction.

LGTM, Manuel does not mind having this and the string heuristic does look like the easiest approach to convey this information in the API.

One more LGTM.

Nice! A more specific name and a more specific library.
LGTM!

Per @ioeric's suggestion: why not move the helper into Tooling/Refactoring/ExtendedRange.h?
If it's in ToolingRefactoring, both stencil and transformer can access it.

In D59376#1454748, @ymandel wrote:

I propose that I create a new library in Core with getExtendedRange() and remove it from FixIt. The other utility functions that I need will also go there. We can separately investigate moving the remaining pieces of FixIt into Core.

LGTM

I'll leave the LGTM to Manuel. I don't have a strong opinion here, but here are some thoughts.

Sam, thanks for taking a look and the useful comments!

Keeping it only in the cpp file also LG if we don't plan to have other usages for now, but please remove the version from FixIt.h

Given that we don't any usages of getExtendedRange, could you remove it from the Fixit.h and move to a header somewhere in the clangToolingRefactoring (e.g. into Transformer.h itself)?

LGTM with the new changes. Specifying the clang::Expr type explicitly when calling change looks a bit less clear than the original approach, but also looks pretty clear to me.

Leaving a few first comments, have to run.
Will take another look later, sorry about the delay today (also feel free to add more reviewers in case this is urgent).

Will address the rest of the comments later, answering a few questions that popped up first.

Looks great, thanks!

Looks neat, thanks!
A final set of NITs from my side, I don't think any of the comments have to do with the actual design of the library, so expect LGTM in the next round.

I'll take the courtesy and land it since @gribozavr is OOO today.

Still have a few comments to address in TokenCollector and wrt to naming. But apart from this revision is ready for another round.

• Rename various things.
• Update doc comments.
• Search tokens in the tests by spelling, not by kind.
• Add more tests.
• Fix typos.
• Address other comments.

Could you provide the rationale for having NodeID vs just using strings for the binds?
Is this just a more type-safe way to do the same thing or is that actually required to solve a particular problem?

The only real question I have is about returning an error vs an empty transformation in of macros.
The rest are just NITs.

A few naming alternatives, will update the review after addressing other comments.

I've added a FIXME to the class.
Also want to add some basic tests before landing this.

• Move Range into the body of Annotations
• Use triple-slash comments
• Added a FIXME that we might want to change the syntax
• Move the doc comment to the class

• s/macroMacroInvocation/something else...

• Rename macro expansion to macro invocation everywhere
• Tweak comments

LGTM

NIT: maybe remove parentheses from the change description, they seem to only add noise.
Adding @gribozavr who definitely knows more about Unicode :-)

I think this option should be configurable (and off by default) for the transition period. A few reasons to do so:

• Before we have an actual implementation of fallback completions, the current behavior (waiting for the first preamble) actually seems like a better experience than returning empty results.
• Some clients do this kind of fallback on their own (e.g. VSCode), so until we can provide actually semantically interesting results (anything more than identifier-based really, e.g. keyword completions) we would be better off keeping it off.
• We can still test it if it's off by flipping the corresponding flag in the test code.
• Would make rollout easier (clients can flip the flag back and forth and make sure it does not break stuff for them)

Mostly nits and small API questions form my side.
This looks very good overall! I'm planning to take a closer look at the handling of macros and various AST nodes in more detail this week, but the approach looks solid from a higher level.

I'm happy to assess the performance costs of this change if needed (the function is obviously on the hot path), but I hope this won't add any significant performance costs.
Also happy to consider alternative approaches to collect the tokens when preprocessing, the current implementation is complicated (search for 'TokenCollector' in D59887) and my intuition is that moving parts of it to the preprocessor should simplify things considerably.

LGTM! Thanks!

Seeking feedback for this change, mostly interested if this a good place for test helpers like this?
I'm planning to author more tests in clangTooling using this soon, so moving it somewhere clang-specific is an option too.

• Added a comments about CUDA files