Page MenuHomePhabricator

[clang-format] [PR46157] Wrong spacing of negative literals with use of operator
ClosedPublic

Authored by MyDeveloperDay on Jun 1 2020, 8:53 AM.

Details

Summary

https://bugs.llvm.org/show_bug.cgi?id=46157

class A;

void foo(A (*)(const A&, const A&), int);
A operator+(const A&, const A&);

void bar() {
  return foo(operator+, -42);
}

is incorrect formatted putting a space between the - and the 42

return foo(operator+, - 42);

This revision tries to refine the annotating of the OverloadOperator to only mark the comma as an operator if its immediately after the operator keyword and to not mark the closing '(' as an TT_OverloadedOperatorLParen unless its actually a (

This code was assuming the usage of operator too much I suspect and not considering its use as a function ptr.

Probably a fairly rare corner case.

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Jun 1 2020, 8:53 AM
MyDeveloperDay edited the summary of this revision. (Show Details)
thopre added a comment.Jun 1 2020, 9:38 AM

That was a speedy patch! I can confirm that my non reduced testcase (git diff -U0 23ac16cf9bd4cc0bb434efcf6385baf083a2ff7b^ 23ac16cf9bd4cc0bb434efcf6385baf083a2ff7b | clang-format-diff.py -i -p1) is fixed with this patch. Thanks! I don't feel confident reviewing the code unfortunately.

That was a speedy patch! I can confirm that my non reduced testcase (git diff -U0 23ac16cf9bd4cc0bb434efcf6385baf083a2ff7b^ 23ac16cf9bd4cc0bb434efcf6385baf083a2ff7b | clang-format-diff.py -i -p1) is fixed with this patch. Thanks! I don't feel confident reviewing the code unfortunately.

No problems, we can wait for the others.

curdeius added inline comments.Jun 2 2020, 3:03 PM
clang/lib/Format/TokenAnnotator.cpp
981

Shouldn't you check CurrentToken for not being null after a call to consumeToken?? It may happen if operator is at the end of input (even if it's a degenerated case).

Ensure no crash when CurrentToken is nullptr

MyDeveloperDay marked an inline comment as done.Jun 3 2020, 6:18 AM
curdeius accepted this revision.Jun 3 2020, 6:42 AM

LGTM.
Keep in mind I'm not the code owner, I don't know if another approval is required.

This revision is now accepted and ready to land.Jun 3 2020, 6:42 AM
This revision was automatically updated to reflect the committed changes.