Page MenuHomePhabricator

clang-format: Allow optimizer to break template declaration.
ClosedPublic

Authored by Typz on Jan 30 2018, 4:46 AM.

Details

Summary

Introduce PenaltyBreakTemplateDeclaration to control the penalty,
and change AlwaysBreakTemplateDeclarations to an enum with 3 modes:

  • None for automatic (e.g. penalty based) wrapping of template declaration
  • BeforeFunction for always wrapping before functions, and applying penalty before classes (e.g. same as legacy behavior when AlwaysBreakTemplateDeclarations=false)
  • Always for always wrapping (e.g. same as legacy behavior when AlwaysBreakTemplateDeclarations=true)

Diff Detail

Repository
rL LLVM

Event Timeline

Typz created this revision.Jan 30 2018, 4:46 AM
Typz edited the summary of this revision. (Show Details)Jan 30 2018, 4:47 AM
Typz edited the summary of this revision. (Show Details)
Typz updated this revision to Diff 131946.Jan 30 2018, 4:51 AM

fix commit message

djasper edited reviewers, added: sammccall; removed: djasper, klimek.Jan 30 2018, 6:21 AM
Typz updated this revision to Diff 132198.Jan 31 2018, 9:24 AM

Force wrap after multi-line template declaration

Typz updated this revision to Diff 132341.Feb 1 2018, 2:30 AM

If the template declaration spans multiple lines, force wrap before the function/class declaration

Please given an explanation of what you are trying to achieve with this change. Do you intend to set the penalty high so that clang-format does other things first before falling back to wrapping template declarations?

Why treat separate declarations differently wrt. wrapping the template declarations? Will this stop at classes vs. functions? I generally have doubts that this option carries it's weight. Do you have a style guide that explicitly tells you to do this?

Typz added a comment.Feb 28 2018, 7:04 AM

Please given an explanation of what you are trying to achieve with this change. Do you intend to set the penalty high so that clang-format does other things first before falling back to wrapping template declarations?

Why treat separate declarations differently wrt. wrapping the template declarations? Will this stop at classes vs. functions? I generally have doubts that this option carries it's weight. Do you have a style guide that explicitly tells you to do this?

I set the penalty relatively high (200), i.e. the same penalty that I have set for PenaltyBreakAssignment, PenaltyBreakBeforeFirstCallParameter, and PenaltyReturnTypeOnItsOwnLine.
Basically we don't have a preference as to where the break should be, and we found that setting the same penalty for all these parameters (including the new PenaltyBreakTemplateDeclaration) gives really "natural" result: e.g. very close to how a user would optimize the code wrapping for readability.

At the moment AlwaysBreakTemplateDeclarations actually applies only to classes, thus templates are always wrapped before functions (unless it all fits on one line). A simpler change would have been to make AlwaysBreakTemplateDeclarations stick to its name, and let it apply to functions as well: but I figured with would break existing clang-format styles.

I think it's possible that this is just a bug/oversight. But I don't fully understand the case where it is not behaving as you expect. Can you give me an example (config setting + code that's not formatted as you expect)?

Typz added a comment.EditedFeb 28 2018, 7:28 AM

The problem I have is really related to the current AlwaysBreakTemplateDeclarations behavior, which does not apply to functions.
I set it to false, and I get this:

template<>
void aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(
    const bbbbbbbbbbbbbbbbbbb & cccccccccc);

instead of:

template<> void aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(
    const bbbbbbbbbbbbbbbbbbb & cccccccccc);

Then when this is fixed the penalty for breaking after the templates part is hardcoded to 10 (prec::Level::Relational), which was not always wrapping as expected (that is definitely subjective, but that is the beauty of penalties...)

The problem I have is really related to the current AlwaysBreakTemplateDeclarations behavior, which does not apply to functions.
I set it to false, and I get this:

template<>
void aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(
    const bbbbbbbbbbbbbbbbbbb & cccccccccc);

instead of:

template<> void aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(
    const bbbbbbbbbbbbbbbbbbb & cccccccccc);

Then when this is fixed the penalty for breaking after the templates part is hardcoded to 10 (prec::Level::Relational), which was not always wrapping as expected (that is definitely subjective, but that is the beauty of penalties...)

Ah, I see. However, you are misunderstanding what the parameter is meant to do (and I think what the name says). It is controlling whether we "always" break before the template declaration (even if everything would fit on just one line). Setting it to false, i.e. "not always" breaking, does not imply that there is any particular situation in which we need to keep it on the same line.

I understand what you want to achieve, but I don't think it is related to whether this is a function or a class declaration, i.e. clang-format also does:

template <typename T>
class AAAAAAAAAAAAAAAAAAAAAAAAAAAA
    : BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB {};

although the template declaration would easily fit on the same line.

So this change does not seem like the right one to make in order to get the options to be more intuitive and for you to get the behavior you want. I'll try to think about how to achieve that. Do you have any ideas?

Typz added a comment.Feb 28 2018, 9:23 AM

Indeed, seems to apply to classes as well. Maybe I was mislead by my testing, where I did not get the case (possibly because we use ConstructorInitializerAllOnOneLineOrOnePerLine=true, so the continuation indenter only sees "short" class declarations unless breaking the template is required because the name is too long).

So just to be clear, are you saying the whole approach is not the right one, or simply that the "names" of each modes are not?
For the name, maybe something like may be better:

  • Never
  • MultiLineDeclaration, or maybe even MultiLine
  • Always

Indeed, seems to apply to classes as well. Maybe I was mislead by my testing, where I did not get the case (possibly because we use ConstructorInitializerAllOnOneLineOrOnePerLine=true, so the continuation indenter only sees "short" class declarations unless breaking the template is required because the name is too long).

Not sure how that style flag is related to class declarations, but ok ;).

So just to be clear, are you saying the whole approach is not the right one, or simply that the "names" of each modes are not?
For the name, maybe something like may be better:

  • Never
  • MultiLineDeclaration, or maybe even MultiLine
  • Always

Ah, yeah, maybe it's just the name. "AlwaysBreak: Never" and "AlwaysBreak: Always" seem not very intuitive to interpret, though. How about just: No, Yes, MultiLine?

Typz updated this revision to Diff 136557.Mar 1 2018, 9:57 AM

Change options values to No/MultiLine/Yes.

djasper accepted this revision.Mar 22 2018, 9:33 AM

Some last comments, but basically looks good.

include/clang/Format/Format.h
352 ↗(On Diff #136557)

Don't forget to run docs/tools/dump_format_style.py to update the docs.

355 ↗(On Diff #136557)

I think it'd be worth having a case here that would actually be formatted differently with BTDS_MultiLine.

lib/Format/TokenAnnotator.cpp
2248 ↗(On Diff #136557)

Indentation is off

This revision is now accepted and ready to land.Mar 22 2018, 9:33 AM
Typz updated this revision to Diff 147005.May 16 2018, 1:27 AM
Typz marked 3 inline comments as done.

Rebase on latest master & address review comments

This revision was automatically updated to reflect the committed changes.