Page MenuHomePhabricator

clang-format: fix spacing in `operator const char*()`
ClosedPublic

Authored by krasimir on Jan 17 2020, 3:48 AM.

Details

Summary

Revision a75f8d98d7ac9e557b238a229a9a2647c71feed1 fixed spacing for operators,
but caused the const and non-const versions to diverge:

// With Style.PointerAlignment = FormatStyle::PAS_Left:

struct A {
  operator char*() { return ""; }
  operator const char *() const { return ""; }
};

The code was checking if the type specifier was directly preceded by operator.
However there could be comments and const/volatile in between.

Diff Detail

Event Timeline

krasimir created this revision.Jan 17 2020, 3:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2020, 3:48 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
krasimir updated this revision to Diff 238738.Jan 17 2020, 3:53 AM
  • Add more tests
mprobst accepted this revision.Jan 17 2020, 4:00 AM
mprobst added inline comments.
clang/lib/Format/TokenAnnotator.cpp
2729

typo dependent

2734

how many const and volatile may be there, and is their order fixed?

We also have the endsSequence helper on FormatToken that might express the concept better (Left.endsSequence(tok::kw_const, tok::kw_volatile) || ...). YMMV, with the tokens being optional that might lead to too many alternates to check for.

This revision is now accepted and ready to land.Jan 17 2020, 4:00 AM
krasimir updated this revision to Diff 238741.Jan 17 2020, 4:21 AM
  • Address review comments
krasimir updated this revision to Diff 238743.Jan 17 2020, 4:22 AM
  • Fix typo
krasimir marked 3 inline comments as done.Jan 17 2020, 4:24 AM
krasimir added inline comments.
clang/lib/Format/TokenAnnotator.cpp
2734

Thank you! I think const and volatile can only appear once, and not both at the same time.
There are 3 cases to check for with endsSequence which is ok, and it's less code.

krasimir marked an inline comment as done.Jan 17 2020, 4:24 AM
Harbormaster completed remote builds in B44257: Diff 238743.
krasimir updated this revision to Diff 238746.Jan 17 2020, 4:26 AM
  • Fix typo
This revision was automatically updated to reflect the committed changes.

@krasimir Thanks for fixing this, as it looks like I broke it.. its much appreciated

@krasimir @MyDeveloperDay @hans Looks like it is a regression from https://reviews.llvm.org/D72911
and the fix isn't in 10.0rc2.
Should we take it?

hans added a comment.Mar 2 2020, 2:21 AM

@krasimir @MyDeveloperDay @hans Looks like it is a regression from https://reviews.llvm.org/D72911
and the fix isn't in 10.0rc2.
Should we take it?

Seems pretty safe. I've pushed it as 99e5b2ff9df5ca4c7fe13b63f60d953058cd9ca3. Thanks!