My 2 cents here. We should probably try hard to keep the cursor inside braces/parentheses for those patterns.
Having

namespace foo {
  decls
}^ <- cursor here

hurts UX. It would be nice to get the old behavior back somehow.
I still have hopes for the VSCode discussion, maybe they can provide a workaround/willing to change their behavior.
@kadircet also suggested trying something like ${2:decls}$0. Could you check if that works in VSCode?

@erichkeane could you take another look at this?

• Update test, check error messages

LGTM. Please take a look the NIT about landing this as two commits.

• Fix typos and apply suggestions in documentation and comments

• Change test from .fail.cpp to .verify.cpp

I've checked that the change works fine on top of D126907. The bug is still there after D126907 and gets fixed by this.
Also, the merge conflict is actually minimal, no code changes intersect.

In D127487#3573180, @erichkeane wrote:

I'm not quite understanding this yet, so I'll have to take another look early next week. However, I AM intending to get https://reviews.llvm.org/D126907 committed in the next week or so. Could you perhaps see how it interacts with that? Its a sizable, multi-month project that I'd like to make sure doesn't get stuck in a rebase-loop again.

• Do not introduce empty branches on asserts
• Test whether specialization or primary template gets picked

Fixed issue is: 54629

@saar.raz @erichkeane please let me know if you are the right people to review this.
The change might need a little more refactoring, e.g. placement new should probably move to a helper in Sema to align with what clang usually does.

In D125742#3563855, @ppluzhnikov wrote:

Could you commit this?

@ldionne @philnik another friendly ping, please take a look when you have a spare minute.

LGTM. Thanks for the cleanup!

Sorry about the delay. Thanks for the cleanup! I only have a few minor comments here.

@ldionne @philnik friendly ping, could you please take another look?

Answering my own question here: I ended up pointing CC and CXX to Clang instead of GCC when running CMake. This makes lit add the -verify flag to tests.

• Fix test failure

I've update change description and the added test. This should fix failures on buildbots and address the comments.
PTAL.

• Update test comment, only check in C++20 and beyond, add expected-errors

Thanks for the quick response. I have addressed the comments, please take another look.

• Add _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION
• Document the new MACRO and add a release note

@ldionne friendly ping. Just want to make sure this does not fall off your radar.
Let me know if you need more time, no rush here from our side.

• Move test to test/libcxx
• Switch test to 'compile.pass' and remove 'main'

In D125773#3525215, @tahonermann wrote:

So the proposal is that -fheader-modules=parse would parse #include of header unit in the same TU, and import .pcm on import, right?

Per cpp.includep7, "If the header identified by the header-name denotes an importable header ([module.import]), it is implementation-defined whether the #include preprocessing directive is instead replaced by an import directive ([cpp.import]) of the form
import header-name ; new-line"

For Clang, I would expect the implementation-defined behavior to derive from the existence of a module map that nominates a header as being importable.

Richard, thanks for course correcting. I was under impression that header modules are not in the standard, my mistake.

LGTM

NIT: typo in the change description

Randam-access

Random

In D125773#3519048, @sammccall wrote:

LG, but I think this is choosing a (new) public name for clang modules/header modules, so maybe Richard or others might want to weigh in?

• Revert gnu++20 fix
• Always pass -fheader-modules/-fno-header-modules when modules are enabled

Please allow some time for the clangd team to review this before landing.
Changes to filenames used to cause unintended consequences for us before. We switched to using file UIDs since then, but we're still not sure it's not going to break us.

LGTM.

LGTM

LGTM.

Many thanks for improving the running time, this slow test has bugged me since the day I first ran it.
I'd even be happy to remove it completely. It's slow, possibly flaky, so IMO the corresponding check does not carry its weight.

@erichkeane sure, will do.

• add FIXME for a memory leak

In D124606#3484740, @Meinersbur wrote:

In D124606#3479793, @ilya-biryukov wrote:

LGTM

This and the commit (@MForster ) was too hasty. There should have been time for people discussing D124563 and D97625 to react, and should have been added as reviewers. I didn't even see @ilya-biryukov participating in the other discussions.

@labath, ah, sorry, you beat me to it.

In D124606#3480012, @aaronpuchert wrote:

I think this change broke again what D97625 was trying to fix. These are text files and we want them to keep the specified line endings regardless of the local git configuration. See also @smeenai's comment D124563#3478625.

NIT: change the commit message to say -text instead of binary

LGTM

• Mention the AutoType traversal does not traverse the constrained concept decl anymore and add a test for it

• clang-format the code

• Check 'Concept auto' completion,
• set -std=c++20 unconditionally,
• use char instead of int.

In D124441#3474123, @sammccall wrote:

This looks great!

Only thing I'm uncomfortable with is the lack of test coverage outside clangd.
I think the most valuable thing would be to have some direct testing of how RecursiveASTVisitor behaves.

Just to be clear, I feel it is not RecursiveASTVisitor that's at fault here. It's rather design of the Index library that splits work into multiple visitors.
Requires expression is produces a template parameter list that suddenly appears out of nowhere in the BodyVisitor, whereas the code to handle it is in the DeclVisitor.

Yeah, that sounds promising. Various source locations inside the same nodes are probably very close. Probably even across nodes.
If we are willing to take on a bit more complexity, we could probably get even bigger wins by storing all source locations in a single array having indices into that array in the actual positions where we need the source locations. That way we can have:

• get the benefits of delta-encoding in the source locations array, probably even across nodes,
• probably still get quite good encoding for indices inside this array inside the actual AST nodes. Will definitely get those if we delta-encode the indices inside the same node.

I suspect there are requirements that will force us to store the array in chunks and allow searching for a particular chunk.

Overall LGTM, but I didn't look closely if there are cases that break now.
Do we have any tests to check this?

LGTM. Nice savings

Added a few minor changes before committing:

• surround names of .clang-format files with double backticks
• ensure no lines are longer than 80 characters