[clang-format] change format for constructor init and switch/case indent

[phunkyfish]

Description

Proposing a new format for constructor init lists. Colon on newline and break after comma for longer lines.

For Short lines is would look like this having everything on one line (a short line is defined by the ColumnLimit in .clang_format, current value is 100:

CPVRChannelGroupInternal::CPVRChannelGroupInternal(bool bRadio) : m_iHiddenChannels(0)
{
}

And for longer lines:

CPVRChannelGroup::CPVRChannelGroup(bool bRadio,
                                   int iGroupId,
                                   const std::string& strGroupName,
                                   const std::shared_ptr<CPVRChannelGroup>& allChannelsGroup)
    : m_bRadio(bRadio),
      m_iGroupId(iGroupId),
      m_strGroupName(strGroupName),
      m_allChannelsGroup(allChannelsGroup)
{
  OnInit();
}

Also proposing an indent for switch/case statements:

switch (m_vecCuts[i].action)
{
  case CUT:
    cutCount++;
    break;
  case MUTE:
    muteCount++;
    break;
  case COMM_BREAK:
    commBreakCount++;
    break;
}

Also updated wording for c++14 usage.

Motivation and Context

The current format does not read well and is a bit ugly.

E.g.:

CPVRChannelGroup::CPVRChannelGroup(bool bRadio,
                                   int iGroupId,
                                   const std::string& strGroupName,
                                   const std::shared_ptr<CPVRChannelGroup>& allChannelsGroup)
    : m_bRadio(bRadio)
    , m_iGroupId(iGroupId)
    , m_strGroupName(strGroupName)
    , m_allChannelsGroup(allChannelsGroup)
{
  OnInit();
}

Same goes for switch/case:

switch (m_vecCuts[i].action)
{
case CUT:
  cutCount++;
  break;
case MUTE:
  muteCount++;
  break;
case COMM_BREAK:
  commBreakCount++;
  break;
}

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• None of the above (please explain below)

Checklist:

• My code follows the Code Guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have updated the documentation accordingly
• I have read the Contributing document
• I have added tests to cover my change
• All new and existing tests passed

[ksooo]

Thanks for picking this up.

[phunkyfish]

Ok, updated as per review.

[ksooo]

👍

[AlwinEsch]

Looks for me better too.

[Rechi]

The reason I like the following style (BreakConstructorInitializers: BeforeComma) is that in most cases it doesn't touch the previous line to add a comma if another initializer is added. If the majority prefers to keep the comma on the previous line that's okay for me.

MyClass::CMyClass(bool bBoolArg,
                  int iIntegerArg,
                  const std::string& strSomeText,
                  const std::shared_ptr<CMyOtherClass>& myOtherClass)
  : m_bBoolArg(bBoolArg)
  , m_iIntegerArg(iIntegerArg)
  , m_strSomeText(strSomeText)
  , m_myOtherClass(myOtherClass)

[ksooo]

Is "doesn't touch the previous line to add a comma if another initializer is added" really worth this very special formatting style? My personal opinion: It's not.

[yol]

How is it "very special"?

[ksooo]

Only my personal feeling, which got inspired from other C++ projects I know.

[garbear]

I think Rechi has a good point about keeping line history clean. However, I'm also with ksooo on this specific case, where I feel the leading comma is ugly enough to warrant an exception to the clean line mantra.

[Montellese]

I strongly prefer the leading comma because it provides much cleaner diffs.

[Paxxi]

The original clang-format was designed to minimize the formatting changes it would do to existing code.
If we're ok with larger changes being made then I don't see any real issue with these changes.

I personally prefer the comma before format as it lines up nicely with the colon. It also has the nice property of not touching the previous line when changing something.

If there's consensus on this style then go for it 😀

[ksooo]

If we're ok with larger changes being made then I don't see any real issue with these changes.

👍

[phunkyfish]

I can also update for guidelines for cpp14 as we support since #13034, right?

[phunkyfish]

Changed wording around c++14 usage and updated additional review points raised.

[Paxxi]

BTW did you know that git blame has an ignore revision option? I found this out recently and I think it could help getting over the reluctance to clean up the whole repos formatting.

It could be done manually or we could add a file that lists cosmetic revisions that one can point to to ignore them

[phunkyfish]

@Paxxi if the git blame option could be set on a repo level that would be cool.

[Paxxi]

It can't afaik. Best we can do is include a file with the commits and then tell people to make an alias or manually use

--ignore-revs-file <file>
Ignore revisions listed in file, which must be in the same format as an fsck.skipList. This option may be repeated, and these files will be processed after any files specified with the blame.ignoreRevsFile config option. An empty file name, "", will clear the list of revs from previously processed files.

[Rechi]

Now one has to open another file to get the value of ColumnLimit. Why not add the maximum line length to the code guidelines?
From where to where is the number of characters actually counted to compare with ColumnLimit?
Will the maximum line length be only used to decide if the everything goes into one line for constructors and ignored everywhere else?

[Paxxi]

Does it matter to spend effort on the coding guidelines that touches formatting?

Nobody should have to care about reading it or thinking about the formatting.

The columnlimit is currently 100. I find it's a decent line length while still fitting two editors side by side on a 1080p screen for diffs. It does cause issues with trailing comments because clang-format doesn't know how to break a comment and does unnatural formatting to fit the comment.

The columnlimit is from 0-100, it doesn't care about indentation and such and is applied everywhere

[phunkyfish]

I could state in Section 1 what the columnlimit is and where it's applied. Then the constructor section can remain clean.

[phunkyfish]

Ok, definition of line length at start of formatting section now.

[yol]

I'm mostly indifferent. The current style is (in the cited points) not inferior to me, but I won't stand in the way if people have a strong opinion/preference.

A thing I don't particularly like is the putting everything on one line thing. It makes it a tiny bit harder to see that a constructor initialization is taking place (since it is at the end of the line some times and in the next line other times).

[phunkyfish]

That’s a fair point. This is what I aimed for initially however it’s doesn’t appear to be possible with the current version of clang-format.

[ksooo]

The current style is (in the cited points) not inferior to me, but I won't stand in the way if people have a strong opinion/preference.

Thanks for that.

A thing I don't particularly like is the putting everything on one line thing.

Same here, but as @phunkyfish said currently not possible with clang-format.

[phunkyfish]

Will merge this tomorrow night unless there are any objections.

[garbear]

I think this PR is the right approach. Thanks @phunkyfish!

[AlwinEsch]

One thing I have to say, unfortunately, which brings a little confused.
Is about value setting in single line. In the context of the example given above, full OK (if one value is set).

Only at the moment clang-format jumps pretty much back and forth between the lines.

e.g.:

MyClass::CMyClass(bool bBoolArg,
                  int iIntegerArg,
                  const std::string& strSomeText,
                  const std::shared_ptr<CMyOtherClass>& myOtherClass)
  : m_bBoolArg(bBoolArg),
    m_iIntegerArg(iIntegerArg),
    m_strSomeText(strSomeText),
    m_myOtherClass(myOtherClass)
{
}

becomes by one value removal, changed to:

MyClass::CMyClass(bool bBoolArg,
                  int iIntegerArg,
                  const std::string& strSomeText,
                  const std::shared_ptr<CMyOtherClass>& myOtherClass)
  : m_bBoolArg(bBoolArg), m_iIntegerArg(iIntegerArg), m_strSomeText(strSomeText)
{
}

Would the one-line also refer to the fact that it is only one value and is together with the rest in a row, would be OK (thats why accepted before).

MyClass::CMyClass(bool bBoolArg) : m_bArg(bBoolArg)
{
}

Only with more than one would be always better like this:

MyClass::CMyClass(bool bBoolArg, int iIntegerArg) 
  : m_bArg(bBoolArg), 
    m_iArg(iIntegerArg)
{
}

[Rechi]

@AlwinEsch clang-format 9 and newer have a new option, AllowAllConstructorInitializersOnNextLine, which makes your expected style possible.
see https://clang.llvm.org/docs/ClangFormatStyleOptions.html

[phunkyfish]

This is a good improvement. However something like this would still be possible:

MyClass::CMyClass(bool bBoolArg, int iIntegerArg) : m_bArg(bArg), m_iArg(iArg)
{
}

Happy to PR this tomorrow. No changes would be required to the docs as this addition will make what's in the doc true always.

Nice find @Rechi.

[phunkyfish]

PR: #16962