Page MenuHomePhabricator

[Format] Fix incorrect pointer/reference detection
ClosedPublic

Authored by Nuullll on Jun 4 2021, 2:42 AM.

Details

Summary

https://llvm.org/PR50568

When an overloaded operator is called, its argument must be an
expression.

Before:

void f() { a.operator()(a *a); }

After:

void f() { a.operator()(a * a); }

Signed-off-by: Yilong Guo <yilong.guo@intel.com>

Diff Detail

Event Timeline

Nuullll requested review of this revision.Jun 4 2021, 2:42 AM
Nuullll created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2021, 2:42 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay accepted this revision.Jun 4 2021, 4:45 AM
This revision is now accepted and ready to land.Jun 4 2021, 4:45 AM
MyDeveloperDay added inline comments.Jun 4 2021, 4:46 AM
clang/lib/Format/TokenAnnotator.cpp
243

feels like we are not testing this situation, please add those tests

clang/unittests/Format/FormatTest.cpp
8783

can you add

void f() { a.operator()(*a * *a); }

Nuullll updated this revision to Diff 350519.Jun 8 2021, 12:45 AM

Add more tests.

Nuullll marked an inline comment as done.Jun 8 2021, 12:51 AM
Nuullll added inline comments.
clang/lib/Format/TokenAnnotator.cpp
243

I added some in FormatTest.UnderstandsOverloadedOperators

clang/unittests/Format/FormatTest.cpp
8283–8286

This patch doesn't fix these, i.e. when operators are called as non-member functions.
The call sites seem to be marked as function declarations.

Pre-check failure: the github server is currently down.

MyDeveloperDay accepted this revision.Jun 8 2021, 4:15 AM
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
236

Do we need to worry about Prev ever being null? Does the assert ever fire? if not why have it in the first place?

We'll crash if it is, do we want to guard against that?

curdeius accepted this revision.Jun 8 2021, 5:09 AM

LGTM modulo nits.

clang/lib/Format/TokenAnnotator.cpp
232–244

Nit: full-stop at the end.

236

Disclaimer: I'm in the pro-assert camp :).
In general, I'd preferably keep this assert because people sometimes use Release+Assertions build. Even more those that report bugs.
And it's much nicer and easier to fix an assertion failure that is well defined and precisely localised than a less clearer report of a crash.
Of course, in a build without assertions, it will crash if Prev is null, but still it will be easier to find the bug when trying to reproduce.

For this specific case, this assertion should never fire because otherwise it would mean that we detected had Left being TT_OverloadedOperatorLParen but there was no corresponding tok::kw_operator (or just operator). So it would mean that there's a bug in (probably) token annotator. So yeah, it falls into "should never happen" category about which assertions are all about IMO.

clang/unittests/Format/FormatTest.cpp
8283–8284

Nit: full-stop.

8283–8286

Is there a bug report on that? If so, please add a link to the comment. If no, could you please create one?
Or is it really something that will never be possible to distinguish (hmm, this would be strange)?

MyDeveloperDay added inline comments.Jun 8 2021, 8:39 AM
clang/lib/Format/TokenAnnotator.cpp
236

To be honest I'm in the assert and guard as well camp (if such a camp exists ;-)

I have this discussion regularly with the other developers in my team, the use assert is useless other than for our assertion builds (which in high performance code, almost no one runs, until there is a problem!)

The assert triggers in my mind that it MIGHT be null (but generally shouldn't) so in which case we have to guard as well for the rare case, but I like the assert.. but please of the form:

assert(condition && "Some nice string telling me exactly what went wrong")

When the assert fires I get a nice error message, not just assert failed with 0

https://llvm.org/docs/CodingStandards.html#assert-liberally

MyDeveloperDay added inline comments.Jun 8 2021, 8:42 AM
clang/unittests/Format/FormatTest.cpp
8283–8286

Might be nice just to fix these as well, this is kind of what happens with clang-format development... you fix one thing we try to make you fix everything else around it, otherwise who is going to understand it better than you! ;-)

We do appreciate your patch!!

Nuullll updated this revision to Diff 350756.Jun 8 2021, 6:30 PM

Minor updates.

Nuullll added inline comments.Jun 8 2021, 6:40 PM
clang/lib/Format/TokenAnnotator.cpp
236

@curdeius @MyDeveloperDay Thanks for the explanations. I've added an error message.

clang/unittests/Format/FormatTest.cpp
8283–8286

@curdeius

Is there a bug report on that? If so, please add a link to the comment. If no, could you please create one?

No. I've created one and added the link in the comment!

Or is it really something that will never be possible to distinguish (hmm, this would be strange)?

I think it should be distinguishable, at least there should be no return type before the operator in the context of non-member function invocation. I'd like to fix this in a separate patch, later. I need some time to figure it out :)

8283–8286

Might be nice just to fix these as well, this is kind of what happens with clang-format development... you fix one thing we try to make you fix everything else around it, otherwise who is going to understand it better than you! ;-)

We do appreciate your patch!!

@MyDeveloperDay Thanks for reviewing this. I will try to fix these in another patch. It's always exciting to contribute to llvm!

MyDeveloperDay accepted this revision.Jun 9 2021, 12:56 AM

Do you need some one to commit this? If yes please state name and email, some one will chime in to commit it.

Do you need some one to commit this? If yes please state name and email, some one will chime in to commit it.

Yes, please someone help commit this.

Name: Yilong Guo
Email: yilong.guo@intel.com

Thanks in advance!

This revision was automatically updated to reflect the committed changes.