This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] fix template closer followed by >
ClosedPublic

Authored by Backl1ght on Jan 2 2023, 7:23 AM.

Details

Summary

This patch is to fix https://github.com/llvm/llvm-project/issues/59785.

related patches: https://reviews.llvm.org/D86581 and https://reviews.llvm.org/D100778.

I think the condition added in D100778 is enough to judge shifts within conditions, the condition added in D86581 is unnecessary and will cause template opener/closer parsed as binary operator, so I simply remove the condition added in D86581.

Diff Detail

Event Timeline

Backl1ght created this revision.Jan 2 2023, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 2 2023, 7:23 AM
Backl1ght requested review of this revision.Jan 2 2023, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 2 2023, 7:23 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I'm good with this but I'd be interesting in @owenpan and @HazardyKnusperkeks opinion.

clang/lib/Format/TokenAnnotator.cpp
169–171

I was wondering this is actually saying....

I think its saying... 2 tokens are next to each other ">>" its not so much formatting but ensuring the existing formatting is maintained, is that what you understand?

Whilst this works it won't correct the case where the whitespace is wrong

i.e. I don't think

if (std::tuple_size_v < T >> 0) {
}

will be corrected to be

if (std::tuple_size_v<T> > 0) {
}

I'm a little wary of rules that use the existing whitespace, but I tend to agree that it might be ok without the extra check.

It would be good to capture this as an annotator test (I like the verifyformat one you put in) but the annotator tests we can assert that its actually a templatecloser and binary operatror

I'm good with this but I'd be interesting in @owenpan and @HazardyKnusperkeks opinion.

Yes, this is what I understand.

And I will add an annotator test later.

This patch is just a temporary solution I think, there are further things to do with such case, maybe I should add a TODO or FIXME?

Backl1ght updated this revision to Diff 486248.Jan 4 2023, 5:21 AM
Backl1ght added inline comments.Jan 4 2023, 5:45 AM
clang/lib/Format/TokenAnnotator.cpp
169–171

I think there should be more infomation to make the correction.

So far, we only know that tuple_size_v is an identifier, but the exact type of tuple_size_v is unknown. Maybe tuple_size_v is a type trait and in this case we should make the correction. Maybe tuple_size_v is an interger and in this case we should not make the correction.

To make the correction, we need at least one of:

  1. is tuple_size_v a type trait?
  2. is T a typename?
  3. is >> a right shift?
MyDeveloperDay accepted this revision.Jan 5 2023, 2:12 AM
MyDeveloperDay added a subscriber: rymiel.

LGTM, thank you for adding an annotator test, I'd like one of @owenpan, @HazardyKnusperkeks or @rymiel to comment before commit, just to get another view.

This revision is now accepted and ready to land.Jan 5 2023, 2:12 AM

I'd like one of @owenpan, @HazardyKnusperkeks or @rymiel to comment before commit, just to get another view.

Perhaps clang-format should never insert a space in >>.

This revision was automatically updated to reflect the committed changes.

I'd like one of @owenpan, @HazardyKnusperkeks or @rymiel to comment before commit, just to get another view.

Perhaps clang-format should never insert a space in >>.

I seem to remember some "quirkiness" with nested templates that need converting

Foo<Bar<T>>

into

Foo<Bar<T> >

but I think that was sometime back.