Page MenuHomePhabricator

[clang-format] Add an option to put one constructor initializer per line
AbandonedPublic

Authored by owenpan on Jun 29 2021, 3:39 AM.

Diff Detail

Event Timeline

owenpan requested review of this revision.Jun 29 2021, 3:39 AM
owenpan created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2021, 3:39 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Seem similar to D90232: [clang-format] Formatting constructor initializer lists by putting them always on different lines (update to D14484) which seems to have got stalled

I sort of feel I prefer the design where we have an enum rather than introducing a separate option.

We already have

AllowAllConstructorInitializersOnNextLine and ConstructorInitializerAllOnOneLineOrOnePerLine

Sort of feels like we need to combine them with this,

I should also say I'd quite like this functionality, its just how do we deliver it via the options that I think we might want to think about, as I sort of feel the 3 options (if we introduced another) are competing with each other.

Seem similar to D90232: [clang-format] Formatting constructor initializer lists by putting them always on different lines (update to D14484) which seems to have got stalled

I sort of feel I prefer the design where we have an enum rather than introducing a separate option.

Yeah I thought, I already saw something like that.

I too think the way with the enum is a better one.

Formatting part and tests look good to me, but I'd rather see this patch merge all related boolean options into one enum.
Just thinking out loud, but is it doable to merge AllowAllConstructorInitializersOnNextLine, ConstructorInitializerAllOnOneLineOrOnePerLine and this patch's ConstructorInitializerAlwaysOnePerLine into e.g. ConstructorInitializerStyle: AllowNextLine | NonBinPack==AllOnOneLineOrOnePerLine | OnePerLine. And one other enum value for the default case without special handling.
Of course, we'd need to keep that backward compatible.

We already have

AllowAllConstructorInitializersOnNextLine and ConstructorInitializerAllOnOneLineOrOnePerLine

Sort of feels like we need to combine them with this,

I should also say I'd quite like this functionality, its just how do we deliver it via the options that I think we might want to think about, as I sort of feel the 3 options (if we introduced another) are competing with each other.

AllowAllConstructorInitializersOnNextLine is like a sub-option of ConstructorInitializerAllOnOneLineOrOnePerLine; it has no effect unless the latter is true. The new ConstructorInitializerAlwaysOnePerLine option, if set to true, overrides ConstructorInitializerAllOnOneLineOrOnePerLine as it should with Always. I think it would be contrived to combine these options in an enum. Is it even practical to do so without breaking the unit tests and backward compatibility?

Seem similar to D90232: [clang-format] Formatting constructor initializer lists by putting them always on different lines (update to D14484) which seems to have got stalled

I sort of feel I prefer the design where we have an enum rather than introducing a separate option.

Yeah I thought, I already saw something like that.

I too think the way with the enum is a better one.

I was aware of that one. Didn't it get blocked because it failed the unit tests? Besides, it didn't handle the AfterColon case.

Formatting part and tests look good to me, but I'd rather see this patch merge all related boolean options into one enum.
Just thinking out loud, but is it doable to merge AllowAllConstructorInitializersOnNextLine, ConstructorInitializerAllOnOneLineOrOnePerLine and this patch's ConstructorInitializerAlwaysOnePerLine into e.g. ConstructorInitializerStyle: AllowNextLine | NonBinPack==AllOnOneLineOrOnePerLine | OnePerLine. And one other enum value for the default case without special handling.
Of course, we'd need to keep that backward compatible.

If we were to start this all over without the need of backward compatibility and the existence of the related unit tests, an enum might be a better option. Then I still think the user might have some trouble relating the following to the enum.

If AlwaysOnePerLine:
  Put each on its own line.
Else If AllOnOneLineOrOnePerline:
  If they all fit on one line:
    Put all of them on a single line.
  Else If AllOnNextLine:
    Put (the rest of) them on the next line.
  Else:
    Put each on its own line.
Else:
  ...

If we were to start this all over without the need of backward compatibility and the existence of the related unit tests, an enum might be a better option. Then I still think the user might have some trouble relating the following to the enum.

If AlwaysOnePerLine:
  Put each on its own line.
Else If AllOnOneLineOrOnePerline:
  If they all fit on one line:
    Put all of them on a single line.
  Else If AllOnNextLine:
    Put (the rest of) them on the next line.
  Else:
    Put each on its own line.
Else:
  ...

Is there anything else in the "Else:" part above? Is there an option that we forgot?

I'm not sure if I understand you correctly. Is your point that an enum option would be hard to find by the users?
But if the doc of each currently existing option pointed to this enum option it would be pretty easy, no?
Or does backward compatibility seem difficult to achieve? It should be a straightforward mapping.

Also, I find that having option that are mutually exclusive (or one that doesn't change anything when another is enabled) is a smell that tells us to use an enum.
Personally I find it harder to finder multiple options that concern the same aspect of the code style rather than a single option.

What I find problematic is clear and descriptive names for all options and possible values, but that's unrelated to whether it's a single enum option or several boolean options.
Having said that, I need to admit that ConstructorInitializerAllOnOneLineOrOnePerLine name makes my blood run cold :).

MyDeveloperDay added a comment.EditedJun 30 2021, 10:23 AM

is a smell that tells us to use an enum.

I totally agree

Personally I find it harder to finder multiple options that concern the same aspect of the code style rather than a single option.

To be honest, I think we know the options some of the best (apart of course from @djasper and @klimek), and most of the time I'm totally confused, as to which options do which, it ends up being trail and error. This way bugs creep in.

Yes I feel that way too this should have been

ConstructorInitializer: Compact

or

ConstructorInitializer: OnePerLine

It would make it easier to add new behavior without over complicating things

Else:
  ...

Is there anything else in the "Else:" part above? Is there an option that we forgot?

No, it's just what you called "one other enum value for the default case without special handling."

I'm not sure if I understand you correctly. Is your point that an enum option would be hard to find by the users?
But if the doc of each currently existing option pointed to this enum option it would be pretty easy, no?

An enum makes it easier for the user to find all related options, but my point is that in this case it would be harder for the user to relate the enum names to the behaviors because of the nested If.

Or does backward compatibility seem difficult to achieve? It should be a straightforward mapping.

Part of backward compatibility is not breaking/changing existing unit tests, which may be difficult to do.

Also, I find that having option that are mutually exclusive (or one that doesn't change anything when another is enabled) is a smell that tells us to use an enum.

You are not suggesting using two enums, one for the outer If and the other for the nested If, right?

If we didn't have AllOnOneLineOrOnePerLine, we could simply use an enum for ConstructorInitializer with three values:

  • OnePerLine (i.e., AlwaysOnePerLine = true)
  • BinPack (i.e., AllOnOneLineOrOnePerLine = OnNextLine = true)
  • Neither (i.e., AlwaysOnePerLine = AllOnOneLineOrOnePerLine = false)