In D58341#1401212, @ilya-biryukov wrote:

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.

I wonder how does merge work with Sema results? See the case below, IIUC our indexer has one symbol for this using decl, but the code completion result from Sema contains two symbols. The symbol ids of these 3 symbols are different, so we will end up with 3 completion results.

That's true, but we're not sure how much this would hurt in practice. Currently we don't show any results from dynamic index for std::strcmp, which is arguably worse than showing an extra completion item for the using.

In D58345#1401040, @ioeric wrote:

Looks great! Thanks for doing this.

Could you also check in the tool that is used to generate the mapping? We need a way to update the mapping when cppreference is updated.

I think we need to update the comment in the SymbolLocation::Column.

I wonder how does merge work with Sema results? See the case below, IIUC our indexer has one symbol for this using decl, but the code completion result from Sema contains two symbols. The symbol ids of these 3 symbols are different, so we will end up with 3 completion results.

Fix style.

The code looks good. For this protocol extension, we need supports from other LSP clients, I think we may want to propose this extension to the LSP specification, so that all LSP servers/clients respect it.

This is an intriguing idea, and is at least useful to prototype new tweaks.

I'm not sure whether clang-tidy is the ultimately right API to write tweaks:

it doesn't have the needed constraints to ensure prepare() is fast (e.g. it emits diagnostics and fixes eagerly)
the exact set of nodes that it will trigger on may or may not match what we want
it doesn't produce context-sensitive titles

Update the comments

Address review comment.

• [clangd] Re-submit format replacement for tweaks.

In D57739#1392453, @sammccall wrote:

In D57739#1390374, @ilya-biryukov wrote:

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.

If providing formatting was free, I'd be fine with this, but it leaks into the public interface in two places: a) it requires the caller to plumb through a formatting style, and b) it is another source of errors.

For this particular interface it's more important that it's conceptually clear and easy to implement than it is that it's easy to call - this is an extension point.

My concerns are code duplication and ease of use for the clients. Having both apply and applyAndFormat (the latter being a non-virtual or a free-standing utility function) in the public interface of the Tweak would totally do it for me.

I'd be happy with applyAndFormat as a free function - my main concern is that formatting not be part of the scope of the class.

The change looks reasonable, I'd wait for a few days before commit in case anyone has concerns.

This is my experiment of playing around tweaks, I think we'll need this.

Update

Remove an unexpected change.

Add reproduce testcase.

update

Fix space.

You'd need to rebase this patch, D57739 had some changes to the Tweak API.

Committed in rL353306.

Remove accident change.

Address comments.

Add missing file.

Address review comments.

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

Thanks!

Move format to the tweak.

Set the flag to false by default.

In D57739#1384844, @ilya-biryukov wrote:

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);
   }

protected:
   virtual Replacements doApply() = 0; // inheritors should implement this now. Note: the name is terrible, suggestions are welcome.
};

This would ensure the callers don't need to deal with the formatting on their own.

Adress review comments.

LGTM

The code looks good, but I have a concern about the check name -- double seems a confusing word, see my comment.

LGTM

I'm +1 on disabling these checks as they add too much noise in editors (even though the naming check is correct, we can't do a global cleanup on the codebase). Let's commit it, we could revert it if someone has other concerns.

Looks good overall, most are nits.

In D57057#1375249, @sammccall wrote:

This turns out to be excessively spammy in unit tests, can we change this to dlog()?

Measured this patch on LLVM, the increasing number of symbols is reasonable (from 422 K to 425 K).