Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[clang-format] Handle shifts within conditions

Authored by Saldivarcher on Aug 25 2020, 5:14 PM.



In some situation shifts can be treated as a template, and is thus formatted as one. So, by doing a couple extra checks to assure that the condition doesn't contain a template, and is in fact a bit shift should solve this problem.

This is a fix for bug 46969

Diff Detail

Event Timeline

Saldivarcher created this revision.Aug 25 2020, 5:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2020, 5:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Saldivarcher requested review of this revision.Aug 25 2020, 5:14 PM
Saldivarcher edited the summary of this revision. (Show Details)
Saldivarcher added a reviewer: Restricted Project.Aug 25 2020, 9:19 PM
Eugene.Zelenko edited reviewers, added: MyDeveloperDay; removed: Restricted Project, Restricted Project.Aug 25 2020, 9:56 PM
Eugene.Zelenko added a project: Restricted Project.

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?

Agreed that this is a very nice solution for this case. LGTM too assuming it passes @MyDeveloperDay's tests.

Minor heads up: there are still some cases I can dream up where we'd currently spit out invalid code from splitting a right shift even after this fix. Long story short, C++14 made some cases completely ambiguous and impossible to detect without semantic analysis ( I'll be having a fix out for that soon, but it's rather complementary to this one-- yours improves template detection, mine acts more conservative in case we guessed wrong about it being a template.

Added some more tests.

MyDeveloperDay, I added your tests. Sorry to step on your toes, I didn't know there was a patch open for this already :(. Yeah, JakeMerdichAMD, it's kinda tough to handle those types of cases without some sort of parsing. I'm glad you're working on that, and my code doesn't get in the way.

MyDeveloperDay accepted this revision.Aug 27 2020, 1:27 AM

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.

This bug has been reported multiple times I think this is a good first step towards a solution, and I don't think "perfection should be the enemy of good", I think for most cases this will suffice.

anything else more obtuse I wonder why people write code in that form when its hard for the non author to read, but we can address those cases when we hit them.

This revision is now accepted and ready to land.Aug 27 2020, 1:27 AM

@Saldivarcher do you have commit access?

@MyDeveloperDay thanks for the kind words, it's much appreciated! I'm just glad I was able to help. Nope, I don't have commit access, can you do me the favor of commiting for me?

This revision was automatically updated to reflect the committed changes.

@Saldivarcher this should be landed now, please validate

@MyDeveloperDay looks like it's in master, thanks for committing for me!