Page MenuHomePhabricator

clang-format Access Modifier Use Normal Indent
Needs ReviewPublic

Authored by LokiAstari on Jul 19 2016, 2:10 AM.

Details

Reviewers
klimek
djasper
Summary

Access Modifiers (public/protected/private) causes standard indent.

class MyClass
{
    int value1;        // standard indent.
    public:            // access modifier (increases indent level)
        int value2;    // next item indent one more level 
    private:           // access modifier goes out a level
        int value3;    // next item indent one more level 
};

Diff Detail

Event Timeline

LokiAstari updated this revision to Diff 64457.Jul 19 2016, 2:10 AM
LokiAstari retitled this revision from to Access Modifier Use Normal Indent.
LokiAstari updated this object.
LokiAstari added reviewers: djasper, klimek.
LokiAstari added a subscriber: cfe-commits.
LokiAstari retitled this revision from Access Modifier Use Normal Indent to clang-format Access Modifier Use Normal Indent.Jul 19 2016, 2:22 AM
djasper edited edge metadata.Jul 19 2016, 8:02 AM

Before we continue with the actual code review and brainstorming how we could actually call this option, can you read through http://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options and provide feedback about why this option qualifies?

LokiAstari added a comment.EditedJul 19 2016, 5:18 PM

Each new style option must ::

  • be used in a project of significant size (have dozens of contributors)
  • have a publicly accessible style guide
  • have a person willing to contribute and maintain patches

Example:

Project:             https://github.com/openframeworks/openFrameworks
Style Guide:         https://github.com/openframeworks/openFrameworks/wiki/oF-code-style#indentation
Contributors:        220
Willing to maintain: Me (Loki Astari. I have more patches I want to contribute so I am not doing a drive bye :-)

@djasper @klimek

Is that a sufficient example? Or do I need to do some more exhaustive search of github for projects?

@djasper @klimek

Hi guys,
I am not sure how to move this PR forward.

I am not sure if you are uninterested or if this is two minor in comparison to other issues.
If there is some other task I need to do to move this forward I more than happy to do some leg work, but given the documentation I am not sure what else I need to do.

Loki.

Sorry, my bad. I should have replied sooner. I believe we can move further with this seems to be used widely enough (though it really doesn't make sense in my personal opinion ;) ).

My main concern is how we are going to configure this in the long run. This is mostly my bad as I was trying to be too clever with the AccessModifierOffset setting.

I see three options:

  1. Roughly go ahead with what you are suggesting, although the option should not be called AccessModifierStandardIndent, as that carries no meaning and actually is far from the "standard" way. Reasonable names that spring to mind are: AccessModifiersCauseAdditionalIndent or (Additional)IndentAfterAccessModifiers.
  2. Be even more "clever" with AccessModifierOffset (I can come up with various ways from negative numbers to special numbers, none of them really good).
  3. Deprecate AccessModifierOffset and instead use an enum with a few dedicated settings.

Manuel, any thoughts?

Hope you don't mind me chiming in:

Roughly go ahead with what you are suggesting, although the option should not be called AccessModifierStandardIndent, as that carries no meaning and actually is far from the "standard" way. Reasonable names that spring to mind are:

Though I personally disagree on the more standard technique :-) Since I am the newbie to this I am not going to fight on that point (though I suspect if we meet up for some beers we could have a spirited debate).

AccessModifiersCauseAdditionalIndent or (Additional)IndentAfterAccessModifiers.

How about "AccessModifiersUseNormaIndent" as we are indenting by a normal indent width. But I am not going to fight over the name when it comes down to it.

Be even more "clever" with AccessModifierOffset (I can come up with various ways from negative numbers to special numbers, none of them really good).

I don't think this is a good idea (but as the newbie no strong feelings). Unfortunately a lot of large projects have stated using this is a standard part of their formatting; got to this project too late to prevent that :-O

Deprecate AccessModifierOffset and instead use an enum with a few dedicated settings.

The issue with this is getting all projects to update their settings with the new version. Not sure this is a great user experience.

@djasper @klimek

Just want to bring this to the top of your queue again :-)
Any further thoughts on the processes?

Loki

klimek edited edge metadata.Aug 2 2016, 5:18 AM

Perhaps call it AccessModifierIntroducesScope or something?

Side-track: I find it highly confusing that in

class C {
  int v1;
  private:
    int v2;
}

v1 & v2 have different indent, although they are in the same scope.

So you'd be for #1 of the three choices from my previous comment?

klimek added a comment.Aug 2 2016, 5:42 AM

So you'd be for #1 of the three choices from my previous comment?

Yes, because I think ultimately introducing scopes is different from simple outdents/indents.

klimek added a comment.Aug 2 2016, 5:42 AM

(full disclosure: I'm also generally opposed to this change, but if there are really enough users using this it's probably a lost cause)

I generally agree. Note, however, that the mentioned style guide doesn't actually specify this. All it says is "The public, protected, and private keywords should be indented inside the class with the function declarations indented as well."

So I guess,

class C {
    int v1;
  private:
    int v2;
};

Would actually also be valid according to the style guide.

I don't have a problem changing it so the default behaviour is:

class C {
    int v1;
  private:
    int v2;
};

But I would like to retain the functionality that if there is no explicit public/private/protected that it follows the original.
But if there is huge objection to that I am not going to fight over this one (I can live with the indention above).

Loki.

I don't have a problem changing it so the default behaviour is:

class C {
    int v1;
  private:
    int v2;
};

But I would like to retain the functionality that if there is no explicit public/private/protected that it follows the original.
But if there is huge objection to that I am not going to fight over this one (I can live with the indention above).

That should already be doable with a negative offset today, right?

That should already be doable with a negative offset today, right?

Yes. So I don't need to add any changes. right?

No, I don't think so. Because you need a "double-indent" within classes.

mithro added a subscriber: mithro.Feb 1 2018, 4:35 PM

I was wondering what the status of this feature was? Looks like it was last touched back in Aug 2 2016 -- do you know if a different or replacement patch was landed? I have a project (https://github.com/verilog-to-routing/vtr-verilog-to-routing) that I'm trying to convince to use clang-format but they use this style of indenting after the accessor modifier and convincing them to change style is a *much* bigger challenge then getting them to adopt clang-format usage :-)