This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Refactor determineStarAmpUsage NFC
ClosedPublic

Authored by sstwcw on Mar 15 2022, 4:26 PM.

Details

Summary

There was some duplicate code in determineStarAmpUsage and
determinePlusMinusCaretUsage

Now a - or + following ;, sizeof, co_await, or delete is
regarded as a unary operator.

Now a * or & following case is also a unary operator.

Diff Detail

Event Timeline

sstwcw created this revision.Mar 15 2022, 4:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 4:26 PM
sstwcw requested review of this revision.Mar 15 2022, 4:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 4:26 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
curdeius requested changes to this revision.Mar 16 2022, 2:28 AM
curdeius added a subscriber: curdeius.
curdeius added inline comments.
clang/lib/Format/TokenAnnotator.cpp
2202–2205

As below, before, question, kw_return, kw_case, at were not handled here.
Please add tests.

2255–2256

I don't like this part of your patch. You remove comments for no real gain.

2256–2260

There were no TT_ConditionalExpr, semi, kw_co_await, kw_delete, kw_sizeof, kw_throw before.
I'd like to see tests for each of them.

This revision now requires changes to proceed.Mar 16 2022, 2:28 AM
HazardyKnusperkeks edited reviewers, added: owenpan, MyDeveloperDay, HazardyKnusperkeks; removed: Restricted Project.Mar 16 2022, 1:28 PM
HazardyKnusperkeks requested changes to this revision.Mar 16 2022, 1:30 PM
HazardyKnusperkeks added inline comments.
clang/lib/Format/TokenAnnotator.cpp
2202–2205

This needs a comment or better a new common function. Otherwise it's just unnecessary hard to understand why we now check for PlusMinusCaret. (I've never heard of this function and I've stepped a lot through clang-format. ;))

sstwcw updated this revision to Diff 416010.Mar 16 2022, 3:51 PM
sstwcw edited the summary of this revision. (Show Details)

Add some test cases and use a separate function for the common parts.

sstwcw marked 3 inline comments as done.Mar 16 2022, 4:00 PM

About the tokens that were only in one function.

question, colon, and TT_ConditionalExpr, are for same thing. question was added before they added TT_ConditionalExpr. It looks like now only TT_ConditionalExpr would be enough.

kw_return and kw_throw were in both functions.

kw_case, kw_co_await, and kw_delete are now only in one function like before this revisioin.

semi and kw_sizeof have test cases.

clang/lib/Format/TokenAnnotator.cpp
2138

I'm not sure about this. Why not handle them here too?

2146–2147

Especially here, why should a + after delete be a binary operator?
How much sense it makes doesn't matter, it is valid c++: https://gcc.godbolt.org/z/c1x1nn3Ej

clang/unittests/Format/FormatTest.cpp
9758

A format test is fine, a token annotator test would be better.

MyDeveloperDay requested changes to this revision.Mar 18 2022, 1:54 AM
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
2146–2147

I'm also trying to understand did you mean you couldn't have

case -1:
case +1:
case +0:
case -0:

https://gcc.godbolt.org/z/qvE44d5xz

This revision now requires changes to proceed.Mar 18 2022, 1:54 AM
sstwcw updated this revision to Diff 416450.Mar 18 2022, 3:50 AM
sstwcw marked 5 inline comments as done.Mar 18 2022, 3:59 AM
sstwcw added inline comments.
clang/lib/Format/TokenAnnotator.cpp
2146–2147

In the new version + following delete is a unary operator. Previously I was under the impression that we only formatted code that is sensible.

2146–2147

case -1:
case +1:

I meant we couldn't have things like case *x or case &x. Is the comment not clear enough?

clang/lib/Format/TokenAnnotator.cpp
2146–2147

case -1:
case +1:

I meant we couldn't have things like case *x or case &x. Is the comment not clear enough?

But we can: https://gcc.godbolt.org/z/8Mb1xo7oP

In the new version + following delete is a unary operator. Previously I was under the impression that we only formatted code that is sensible.

What is sensible this is rather subjective, isn't it? I went a long way to get requires clauses and expressions doing to "right thing" and even added explicitly the unit tests for stuff I think no sane person would ever write, but it's valid code. So in my opinion it should be formatted correctly. I got it wrong on a couple of cases, all but one that I'm aware of are fixed now.

And even if it were invalid code, when it makes the functions to reason about easier why not? I think invalid code can be formatted weirdly, once it's valid again we will format it correctly. But all valid code should be formatted as best as we can.

2146–2147

In the new version + following delete is a unary operator. Previously I was under the impression that we only formatted code that is sensible.

clang/unittests/Format/TokenAnnotatorTest.cpp
85 ↗(On Diff #416450)

I know the other cases also use EXPECT, but the size should be an ASSERT, so there would be no seg fault if the number would change (it shouldn't).

157 ↗(On Diff #416450)

This is the Cast Paren case, right? Please also add the paren check.

162 ↗(On Diff #416450)

The exclaim too?

But we can: https://gcc.godbolt.org/z/8Mb1xo7oP

This is what I love about you all, you push me to learn more about C++, its a slippery b****r

sstwcw updated this revision to Diff 417115.Mar 21 2022, 3:20 PM
sstwcw marked an inline comment as done.
sstwcw edited the summary of this revision. (Show Details)
sstwcw marked 5 inline comments as done.Mar 21 2022, 3:25 PM
sstwcw added inline comments.
clang/unittests/Format/TokenAnnotatorTest.cpp
85 ↗(On Diff #416450)

I tried a wrong number and EXPECT_EQ didn't give a segfault.

sstwcw marked an inline comment as done.Mar 21 2022, 3:25 PM

Just a side note, I often get this on your changes:
Unhandled Exception ("Exception")
Found unknown intradiff source line, expected a line beginning with "+", "-", or " " (space): \ No newline at end of file
.

clang/unittests/Format/TokenAnnotatorTest.cpp
85 ↗(On Diff #416450)

The potential segfault is not in EXPECT_EQ, but in accessing a non existent token.
The number of tokens will most likely never change, but than again we don't need the EXPECT_EQ either.
But if it changes (gets smaller) we want to abort the test after the failed check and not accessing Tokens with an invalid index, that's why I'm pleading (and using) ASSERT_EQ.

sstwcw updated this revision to Diff 419007.Mar 29 2022, 4:23 PM
sstwcw retitled this revision from [clang-format] Refactor determineStarAmpUsage to [clang-format] Refactor determineStarAmpUsage NFC.

I cannot view the diff between the last two revisions:

Unhandled Exception ("Exception")
Found unknown intradiff source line, expected a line beginning with "+", "-", or " " (space): \ No newline at end of file
.

Maybe it's because I removed the final empty line when I pasted the diff as I thought that LF was a line terminator instead of a line separator. I will try using arc from now on.

MyDeveloperDay accepted this revision.Apr 19 2022, 7:00 AM
curdeius accepted this revision.Apr 21 2022, 3:06 AM
This revision is now accepted and ready to land.Apr 21 2022, 3:06 AM
This revision was automatically updated to reflect the committed changes.