Page MenuHomePhabricator

clang-format: Implement AlwaysBreakAfterDeclarationReturnType.
ClosedPublic

Authored by zturner on Jun 10 2015, 4:07 PM.

Details

Summary

AlwaysBreakAfterDeclarationReturnType is like
AlwaysBreakAfterDefinitionReturnType, but inserts breaks
before declaration identifiers, not definition identifiers.

AlwaysBreakAfterDeclarationReturnType defaults to false
(old behaviour) for all styles.

Diff Detail

Event Timeline

strager updated this revision to Diff 27471.Jun 10 2015, 4:07 PM
strager retitled this revision from to clang-format: Implement AlwaysBreakAfterDeclarationReturnType..
strager updated this object.
strager edited the test plan for this revision. (Show Details)
strager added a reviewer: djasper.
strager added subscribers: sas, abdulras, Unknown Object (MLST).
djasper added inline comments.Jun 11 2015, 12:59 AM
include/clang/Format/Format.h
278–281

Hm.. I wonder whether we should really have two separate flags here or a single flag with an enum value, e.g. AlwaysBreakAfterReturnType with the values (No, DefinitionOnly, All).

Not sure we'll also need DeclarationOnly, but I don't think so.

lib/Format/ContinuationIndenter.cpp
129–130

I find the parentheses confusing.

lib/Format/TokenAnnotator.cpp
1546

Call this IsDefinition.

strager added inline comments.Jun 11 2015, 2:15 PM
include/clang/Format/Format.h
278–281

That would break all .clang-format files with AlwaysBreakAfterDefinitionReturnType specified, yes?

lib/Format/ContinuationIndenter.cpp
129–130

What's a better way to write this?

lib/Format/TokenAnnotator.cpp
1546

Will do.

strager planned changes to this revision.Jun 11 2015, 2:20 PM
djasper added inline comments.Jun 11 2015, 9:16 PM
include/clang/Format/Format.h
278–281

Well, the corresponding old strings in the configuration file can still set the right enum value. In that way, this should be fully backwards compatible.

lib/Format/ContinuationIndenter.cpp
129–130

I would just do:

if (Current.is(TT_FunctionDeclarationName) && State.Column < 6 &&
    !Style.AlwaysBreakAfterDefinitionReturnType &&
    !Style.AlwaysBreakAfterDeclarationReturnType)
strager added inline comments.Jun 12 2015, 11:43 AM
include/clang/Format/Format.h
278–281

So would there be two members in Style, or one? What happens when both are specified? Do any other flags behave this way?

lib/Format/ContinuationIndenter.cpp
129–130

Looking at this a bit more, we should probably do the same thing we do in TokenAnnotator::calculateFormattingInformation. How should I factor out the IsDefinition calculation?

strager updated this revision to Diff 27604.Jun 12 2015, 2:46 PM

Fix test diff on Phabricator.

strager planned changes to this revision.Jun 12 2015, 2:48 PM
strager updated this revision to Diff 28753.Jun 29 2015, 10:46 PM

Rebase. Refactor option to be a ReturnTypeBreakingStyle,
similar to AlwaysBreakAfterDefinitionReturnType.

djasper added inline comments.Jul 12 2015, 12:22 AM
docs/ClangFormatStyleOptions.rst
221 ↗(On Diff #28753)

Do you think it'll ever make sense to break differently for declarations and definitions? I think having two entirely independent configuration flags gives us 9 combinations (assuming the three enum values will remain) out of which many will never be used.

I see two alternatives:
Add an additional bool flag "TreatDeclarationsLikeDefinitions" (name might not be ideal yet).
Change existing flag name to AlwaysBreakAfterReturnType and use five enum values (None, TopLevel, All, TopLevelDefinitions, AllDefinitions).

What do you think?

Sorry for being very picky on this. The problem is that these options stick around for a long time and we need to think about them carefully.

lib/Format/ContinuationIndenter.cpp
129–132

What do you mean exactly?

(Sorry for the late feedback.)

docs/ClangFormatStyleOptions.rst
221 ↗(On Diff #28753)

I think having two entirely independent configuration flags gives us 9 combinations (assuming the three enum values will remain) out of which many will never be used.

I agree that many combinations will be unused. I don't see why we should conflate the two if they are configured independently in practice, though.

Add an additional bool flag "TreatDeclarationsLikeDefinitions" (name might not be ideal yet).
Change existing flag name to AlwaysBreakAfterReturnType and use five enum values (None, TopLevel, All, TopLevelDefinitions, AllDefinitions).

What about styles where declarations have return types on their own line, but definitions don't? I don't see a reason to prevent that style.

lib/Format/ContinuationIndenter.cpp
129–132

See IsDefinition in lib/Format/TokenAnnotator.cpp introduced by this diff. The logic here and there should match, I think.

djasper added inline comments.Sep 17 2015, 2:00 AM
docs/ClangFormatStyleOptions.rst
221–235 ↗(On Diff #34947)

Same as I am arguing on some of your other patches. Fewer options are easier to maintain and easier to discover.

include/clang/Format/Format.h
133–136

Only one member in style, but parsing the configuration file should be backwards compatible. If both are present in the configuration file, the non-deprecated one should win. There is a few examples of where the configuration options are parsed for backwards compatibility, e.g. DerivePointerBinding.

lib/Format/ContinuationIndenter.cpp
129–132

You mean just the "!Line.Last->isOneOf(tok::semi, tok::comment);"? You can put this into a member function of AnnotatedLine maybe?

strager planned changes to this revision.Sep 17 2015, 1:21 PM
strager added inline comments.Sep 17 2015, 2:47 PM
docs/ClangFormatStyleOptions.rst
221–235 ↗(On Diff #34947)

I think having separate options for separate cases is easier to maintain.

Current method (separate option):

FormatStyle::ReturnTypeBreakingStyle BreakStyle =
    Line.mightBeFunctionDefinition()
        ? Style.AlwaysBreakAfterDefinitionReturnType
        : Style.AlwaysBreakAfterDeclarationReturnType;
if ((BreakStyle == FormatStyle::DRTBS_All ||
     (BreakStyle == FormatStyle::DRTBS_TopLevel && Line.Level == 0)))
  Current->MustBreakBefore = true;

Proposed method:

auto BreakStyle = Style.AlwaysBreakAfterReturnType;
if (BreakStyle == FormatStyle::DRTBS_All ||
    (BreakStyle == FormatStyle::DRTBS_TopLevel && Line.Level == 0) ||
    (!Line->mightBeFunctionDefinition() &&
     (BreakStyle == FormatStyle::DRTBS_AllDeclarations) ||
     (BreakStyle == FormatStyle::DRTBS_TopLevelDeclarations &&
      Line.Level == 0)))
  Current->MustBreakBefore = true;
strager updated this revision to Diff 35038.Sep 17 2015, 2:55 PM

Fix missing IsDefinition check in ContinuationIndenter::canBreak.

djasper edited edge metadata.Sep 21 2015, 9:36 AM

Maintenance is not about writing 7 lines of code correctly, but ensuring that all these options work correctly and in combination with all other options and that options remain discoverable and well documented. So, please change this to using the one enum with 5 configuration values.

dawn added a subscriber: dawn.EditedOct 2 2015, 1:04 PM

Can someone please accept and commit this patch so we can use this feature? (After the objections are fixed of course). Thanks.

No. I really don't want to have these two completely independent flags, so the patch needs changes, before it can go in.

What needs to happen for this to go in? If I understand correctly, it is either:

  1. Add a new option TreatDeclarationsLikeDefinitions
  2. Merge this option into AlwaysBreakAfterDefinitionReturnType and make it an enum with 5 values.

In theory I think option 2 is the best, but are you concerned about backwards compatibility? It breaks anyone already using AlwaysBreakAfterDefinitionReturnType and forces them to update their style files. Is this an important consideration?

I don't really like option 1 because then we are tied to treating declarations like definitions for all current and future options including ones we haven't imagined yet. That might not be the best choice.

Let me know what to do, and I will finish out this patch if strager won't.

I think we can add the option AlwaysBreakAfterReturnType but still be backwards compatible. If clang-format finds only the old option in a configuration file, it can choose the appropriate value of the new option.

I also like using just this one option better than using the one option with the added bool option.

I think we can add the option AlwaysBreakAfterReturnType but still be backwards compatible. If clang-format finds only the old option in a configuration file, it can choose the appropriate value of the new option.

I also like using just this one option better than using the one option with the added bool option.

What if it finds both the old option and the new option, but they conflict? Error out?

I'd just prefer the setting of the new option and ignore the old one then, but I don't think it matters to much. Warning or err'ing out also seems like a reasonable approach.

zturner commandeered this revision.Dec 14 2015, 2:16 PM
zturner updated this revision to Diff 42774.
zturner added a reviewer: strager.
zturner edited edge metadata.

This patch was very old, so it didn't apply against ToT. So this initial update is just to rebase against ToT. I will make changes in a followup patch.

zturner updated this revision to Diff 42895.Dec 15 2015, 12:53 PM

Attempt to address remaining issues in patch.

This is my first time touching anything having to do with clang, so there's a good chance I don't know what i'm doing and this needs more work. Let me know.

AlwaysBreakAfterDefinitionReturnType is used only as a way to initialize AlwaysBreakAfterReturnType, and is ignored during any actual logic.

djasper accepted this revision.Dec 18 2015, 11:08 AM
djasper edited edge metadata.

Generally looks good.

include/clang/Format/Format.h
166

Use tools/dump_format_style.py to update the documentation, too.

171

nit: top-level

lib/Format/TokenAnnotator.cpp
1642

No braces, I think.

lib/Format/TokenAnnotator.h
168

Some comment might help. E.g. at the very least, does that mean must break before or after "Token" (alternatively, name that Left or Right).

This revision is now accepted and ready to land.Dec 18 2015, 11:08 AM
zturner added inline comments.Dec 18 2015, 11:26 AM
lib/Format/TokenAnnotator.h
168

Turns out this variable wasn't even used. I'll just delete it.

Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in rL256046.