LGTM

it certainly LGTM, like I said I've had met this issue before and I think this does get around that situation where perhaps your not even using the option it's complaining about (especially if its a C++ option and you are using clang-format for C#)

IMHO I feel that given that these changes are more wide reaching than just clang-format that it might be more correct for your to request commit access yourself.

In D87291#2278743, @curdeius wrote:

LGTM.
Don't we risk any other side effects?

In D87587#2276462, @kuzkry wrote:

Thanks @MyDeveloperDay, I appreciate your input in this review. You're a good reviewer. :)
Also, I think you've made some good points there and I'm going to turn this review into "non-breaking the world" one. I'll be back in a few days with a new diff.

ping

Firstly let me say thank you for the patch, and taking the time to listen to the reviewers. Please don't be discouraged by my reply/opinon.

Ok almost...

I have realized this is the wrong change

I have a hard time when people change tests! just because one person wants them this way doesn't mean everyone will.

Minimize other places where try could be incorrectly seen as an identifier

LGTM, thank you for doing this..

LGTM, I'm happy if @JakeMerdichAMD is

Note to self, other things could cause the same issue

@Saldivarcher this should be landed now, please validate

Better solution here D86581: [clang-format] Handle shifts within conditions

Thanks for the patch, I think this looks like a comprehensive improvement, a new nits only

Regarding not touching the LLVM support library: I'd love to find a way, but as clang-format uses the >> operator

Thanks for the patch, You need to generate a fill context diff (see Contributing to LLVM)

This has caught me out from time to time, but the presence of such an option can lead to users mixing clang-format versions which could lead to flip/flopping of changes, so I'm not 100% sure it won't drive bad behaviour.

LGTM

LGTM, I think we need something like this for those keywords/macros

Assuming it builds with minimum required c++ compiler

LGTM

LGTM

This LGTM

I'm not a C++ language lawyer so I'm not sure which standard introduce enum inheritance? (maybe its something we could always do?), if its a recent change we just need to ensure we are conforming to the C++ version that is needed to build clang.

LGTM

This LGTM, I've certainly been bit myself in the past with not knowing which test actually failed especially when they are all foo() something.. thank you

LGTM

LGTM

I'm wondering if this could be used to help the CUDA usages of clang-format

This LGTM thank you

LGTM thank you for these updates

@Saldivarcher do you have commit access?

I think this LGTM and the solution is much more elegant than mine! You were not stepping on my toes, I'm always happy for someone else to take things over and this is a much smarter solution than mine.

I think I like your solution better than mine D79293: [clang-format] [PR45218] Fix an issue where < and > and >> in a for loop gets incorrectly interpreted at a TemplateOpener/Closer, could you look at my unit tests and consider adding them here?

LGTM

Thank you for the patch, this LGTM, I think this kind of change should help reduce memory usage and I feel improves the readability.

a new minor Nits but looks good.

Thank you for the patch, I'm generally in agreement but I think we should let some others comment

just reopening to formally accept

Thanks for assistance and review. And sorry about the extra noise.

Nit:clang-format the patch

Thanks for doing this..

Let’s do all 4, None, Both, Left, Right

you need to make a full contextdiff git diff -U 99999999

In principle, this looks ok.

please clang-format

clang-format -n warnings before this change in clang/test/Analysis/*.cpp (clang-format -n *.cpp |& grep warning | wc -l)

Apologies our clang-format fix D83564: [clang-format] PR46609 clang-format does not obey `PointerAlignment: Right` for ellipsis in declarator for pack broke the buildbots for Polly. Hope this is ok to fix this.

This issue is caused by the presence of the ; in the requires clause (0)>;) inside the template declaration

https://reviews.llvm.org/D79773#2131680 has something to do with your .clang-format file, the base LLVM style seems fine.

We may not be consistent across all of LLVM

Thank you @JohelEGP please keep them coming, I'll try and update the patch as soon as I have some fixes, I kind of feel like we are addressing what would have just been downstream bugs anyway. (some are quite bizarre!) so lets squish as many of them as we can before we land this.

Addressed review comments
Added one or two more tests

Still a work in progress but covers I think the case @JohelEGP found.

I'd be interested to know if you felt any of those changes were an unwanted surprise

Thank you I see now... super subtle but I'll take a look.

In D79773#2124231, @JohelEGP wrote:

constexpr plane(const plane& other) noexcept(
    (detail::default_allocator_is_nothrow &&
     std::is_nothrow_copy_constructible_v<T>)) requires(std::copyable<T>)
  : plane{to1d(other), other.sz.w}
{
}

Without the parentheses in trailing requires-clauses, all code following the constructor initializer list is further indented.