• Update .gitignore

• Remove trailing slash from the path

• Fix a syntax error in the test

LGTM, but see the comment about avoiding code duplication

LGTM from my side, with a few NITs.
But let's wait for an LGTM from Haojian too, to make sure his concerns are addressed.

LGTM

This LGTM wrt to layering, i.e. it's nice we don't need to change the code of diagnostics.
It's been a while since I've looked at the ASTUnit code, though, would be good if someone else took an extra look at it.

It feels that storing template parameters in the index is a waste, one can only access them through the scope they are introduced in (there are out-of-line definitions of a function, but I think we treat every redeclaration of template headers separately).
Can we get the information from the AST whenever we need it?

In D58341#1401295, @hokein wrote:

std::strcmp is a fair case here. Sema seems not returning using-decls as part of code completion results, it this an intended behavior?

Yeah, I think it is. There's an explicit code path that takes the target decls of a using. Arguably, that's good if you to show signatures of the methods.

For context: @hokein mentioned problems in the clangd's code completion if we would index these symbols.
This patch in itself does not hurt much, users of the indexing API can decide how to deal with UsingDecl on their own, clangd is just one of the clients.

Could we also add a test for code completion to make sure we return the new decls there?

LGTM, but we need to make sure the variable is used in configs without assertions, otherwise we'll break buildbots.

LGTM with a typo-nit :-)

LGTM

LGTM

LGTM

Only have a few NITs, will dig deeper into the change tomorrow.
Added @arphaman too as an owner of the index library. Alex, feel free to reassign if you're the wrong person to take a look at this

• Address comments

LGTM

Still need to figure out why the test fails and fix it before submitting the change.

In D57739#1390321, @sammccall wrote:

It's not about stability or whether the functionality is desired, but layering.
Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.

My argument is that it's better to provide formatting by default in the public interface for applying a single tweak.
I might be missing some use-cases, e.g. one that comes to mind is applying multiple tweaks in a row, in which case we'd want to format once and not for every tweak.

In D57739#1390252, @sammccall wrote:

I agree with this concern, and don't think this is an appropriate layer to be aware of styling or failing on format errors. Can we consider moving the formatting to clangdserver as originally planned?

clang-format seems to be somewhat stable, do we actually expect that to be a large problem in practice?
On the other side, I can't imagine any clients that don't need formatting? E.g. it's nice when tests give the same results as one would see in the final clangd and tests don't go through ClangdServer.

• Simplify code

• Move the string to CIndexer.

LGTM.
NIT about a description: we can't be 100% certain that it's related to hyper-threading, so I'd avoid putting that to the description, maybe simply mention that this avoids higher latency for foreground operations?

Repro:

template <class T>
struct Foo {};

LGTM! Also a few NITs.

In D57814#1386902, @hokein wrote:

This patch updates dev dependencies, please correct the commit title to avoid confusion :)

Update the description to add some context

In D57739#1385144, @hokein wrote:

This seems introduce intrusive changes to the Tweak interface (Tweak will need to know the FormatStyle somehow), I think this might be done in another patch.

It's totally fine, since we have just a single tweak now and changing the interface costs nothing.
The important part is making it hard to misuse the interface and forcing formatting on all the users of the interface is a significant burden. E.g. other users of ClangdServer (tests, users of clangd C++ API) shouldn't worry about formatting on their own.

LGTM. Many thanks!

Could we move the code to the Tweak itself, so that all the callers would get the formatting? I.e.

class Tweak {
   Replacements apply() {  // This is now non-virtual.
     auto Replacements = doApply();
     return cleanupAndFormat(Replacements);
   }

LGTM

The change looks reasonable to me, given that you can change the default language version via a build argument.
I don't have the required experience with the lit-tests to give proper approval, though, would wait for someone more relevant to do this.

LGTM.
See the NIT about llvm::, though. Also adding a test with multiple selected ifs would be reasonable, to document and validate behaviour in such cases.

LGTM and sorry for delaying this with discussions.
The change looks solid, regardless of potential problems we may still run into.

• Address comments

@arphaman, thanks for pointing this out. It's definitely nice to change stuff only in one place.
There are two issues still pending:

• index-file.cu still fails, I haven't looked into it closely yet, any idea why that might be happening?; repro is simple, just patch this in locally and run the test.
• I'm not sure it's safe to use temporary storage for argv[0] in clang_parseTranslationUnit2. The resulting translation units probably holds references to argv and is free to read the data later. A simple and safe alternative would be to store this string in CIndexer and return a reference here. Does that LG?

• Revert "changes to main"
• Compute the path in parseTranslationUnit2

In D57570#1380327, @sammccall wrote:

Maybe we should consider stopping the traversals up at some points, e.g. at lambda decls or something similar to limit this class of problems in the future?

If you mean in some frameworky sort of way... yeah, it's quite likely we need something. At least to make the Node* -> DynTypedNode -> Stmt -> IfStmt dance less clunky.
I've deliberately avoided adding too much in the way of helpers for now in favor of the raw data structures, mostly wanting to see what the common patterns are - we can afford to write a few of these by hand.

Exactly, the code dealing with typed nodes there is definitely too verbose. Thinking about helpers like findAncestor<IfStmt>().
And yeah, totally fine to keep it as is for now, it's great that the change is focused.

The "select everything in a very large file" is exactly the problematic case that came to mind before. I'm sure it's possible to build the selection tree in linear time, but the traversals might also become a problem.

• Also disable 'misc-non-private-member-variables-in-classes' in llvm/.clang-tidy

I wonder if this makes the check available in too many places?

Awesome job. Any data on how much time this takes to build? Do you think this would ever be a bottleneck?

• Remove Dummy.cpp
• Add halfOpenRangeTouches
• Add a comment about file vs expansion locations
• Move range manipulations with else and then to apply()
• Remove test fixture, turn test member functions into free functions
• Add checkTransform
• Replace a null check for getCond() with an isValid() check for the corresponding location.

I'm not sure who the owners of c-index-test are, so adding people who touched the file recently or might be interested in this landing.
Suggestions for other reviewers are very welcome.

I don't think it's pure noise. Vscode displays diagnostics in the "PROBLEMS" tab. A suffix allows you to tell whether fixes are available without hovering on the errors.

And it shows bulb icons when you hover over diagnostics in this dialog too. To be clear, it's my personal preference to not have it, that's the reason I call it 'noise', others might like the message and it feel more useful to them.

In D57509#1378943, @ioeric wrote:

Yes, but a new option seems a bit of an overkill here. The text is appended and doesn't seem to affect the readability of original diagnostic message much (IMO). Could you elaborate why this is undesirable in vscode for you?

It's basically just noise, as VSCode shows the bulb icons whenever you hover over the diagnostics with errors.
Having a standard suffix in half of the diagnostics seems undesirable.

LGTM, just a few nits

Could this be made optional for VSCode?
As mentioned in the discussion before, I would personally prefer to turn it off, even though I do acknowledge the need for this for some clients, e.g. vim.

• Update license header

• Update the license header

• Update license header

• Update license headers in new files
• Add an empty cpp file to avoid cmake errors due to empty inputs
• clang-format
• Update the 'fixits-command.test' to unbreak it (the line number was larger than the number of lines in the file)