Page MenuHomePhabricator

[clang-format] Update noexcept reference qualifiers detection
ClosedPublic

Authored by krasimir on Oct 9 2019, 5:13 AM.

Details

Summary

r373165 fixed an issue where a templated noexcept member function with a
reference qualifier would be indented more than expected:

// Formatting produced with LLVM style with AlwaysBreakTemplateDeclarations: Yes

// before r373165:
struct f {
  template <class T>
      void bar() && noexcept {}
};

// after:
struct f {
  template <class T>
  void bar() && noexcept {}
};

The way this is done is that in the AnnotatingParser in
lib/FormatTokenAnnotator.cpp the determination of the usage of a & or &&
(the line in determineTokenType

Current.Type = determineStarAmpUsage(...

is not performed in some cases anymore, combining with a few additional related
checks afterwards. The net effect of these checks results in the & or &&
token to start being classified as TT_Unknown in cases where before r373165
it would be classified as TT_UnaryOperator or TT_PointerOrReference by
determineStarAmpUsage.

This inadvertently caused 2 classes of regressions I'm aware of:

  • The address-of & after a function assignment would be classified as TT_Unknown, causing spaces to surround it, disregarding style options:
// before r373165:
void (*fun_ptr)(void) = &fun;

// after:
void (*fun_ptr)(void) = & fun;
  • In cases where there is a function declaration list -- looking macro between a template line and the start of the function declaration, an & as part of the return type would be classified as TT_Unknown, causing spaces to surround it:
// before r373165:
template <class T>
DEPRECATED("lala")
Type& foo();

// after:
template <class T>
DEPRECATED("lala")
Type & foo();

In these cases the problems are rooted in the skipping of the classification of
a & (and similarly &&) by determineStarAmpUsage which effects the formatting
decisions later in the pipeline.

I've looked into the goal of r373165 and noticed that replacing noexcept with
const in the given example produces no extra indentation with the old code:

// before r373165:
struct f {
  template <class T>
  int foo() & const {}
};

struct f {
  template <class T>
      int foo() & noexcept {}
};

I investigated how clang-format annotated these two examples differently to
determine the places where the processing of both diverges in the pipeline.
There were two places where the processing diverges, causing the extra indent in
the noexcept case:

  1. The const is annotated as a TT_TrailingAnnotation, whereas noexcept is annotated as TT_Unknown. I've updated the determineTokenType function to account for this by adding a missing tok:kw_noexcept to the clause that marks a token as TT_TrailingAnnotation.
  2. The & in the second example is wrongly identified as TT_BinaryOperator in determineStarAmpUsage. This is the reason for the extra indentation -- clang-format gets confused and thinks this is an expression. I've updated determineStarAmpUsage to check for tok:kw_noexcept.

With these two updates in place, the additional parsing introduced by r373165
becomes unnecessary and all added tests pass (with updates, as now clang-format
respects the style configuration for spaces around the & in the test
examples).
I've removed these additions and added regression tests for the cases above.

Diff Detail

Event Timeline

krasimir created this revision.Oct 9 2019, 5:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 5:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
krasimir marked an inline comment as done.Oct 9 2019, 5:18 AM
krasimir added inline comments.
unittests/Format/FormatTest.cpp
7093 ↗(On Diff #224022)

note: the old test case was identical to the one above, I think the original intent was to check both & and && cases, so modified accordingly.

Only because I had problem finding the commit, this is as a consequence of D68072: [clang-format] Reference qualifiers in member templates causing extra indentation.

MyDeveloperDay accepted this revision.Oct 9 2019, 5:45 AM

Thank you for fixing this, I'm sorry my review probably wasn't thorough enough in the first place, this LGTM and thank you for the very thorough description as this really helps me see.

On a side rant, for me this only adds fuel to my feeling that we need D68554: [clang-format] Proposal for clang-format to give compiler style warnings and D29039: [clang-format] Proposal for clang-format -r option in order to be able to run new clang-format binaries over a more substantial preformatted codebase quickly and easily, prior to commit of any changes.

lib/Format/TokenAnnotator.cpp
1499 ↗(On Diff #224022)

I slightly wonder if other training annotations like override and final might also suffer simular to what we saw with D68481: [clang-format] [PR27004] omits leading space for noexcept when formatting operator delete()

unittests/Format/FormatTest.cpp
7093 ↗(On Diff #224022)

nice catch

This revision is now accepted and ready to land.Oct 9 2019, 5:45 AM
MyDeveloperDay added a project: Restricted Project.Oct 9 2019, 5:48 AM
krasimir marked 3 inline comments as done.Oct 9 2019, 6:01 AM
krasimir added inline comments.
lib/Format/TokenAnnotator.cpp
1499 ↗(On Diff #224022)

I'll check these cases and add appropriate tests etc. in a subsequent patch (maybe next week).

I think that part of the issue is that the trailing annotations back in the day where this code was written (7 years ago judging by a few blames) were captured by the tok::identifier case; however since then C++ and clang have evolved a lot and have extracted separate tokens for these, hence the missing cases.

MyDeveloperDay added inline comments.Oct 9 2019, 6:15 AM
lib/Format/TokenAnnotator.cpp
1499 ↗(On Diff #224022)

Sound good..

This revision was automatically updated to reflect the committed changes.
krasimir marked an inline comment as done.