Page MenuHomePhabricator

[clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine
Needs ReviewPublic

Authored by Manikishan on Jun 9 2019, 12:21 PM.

Details

Summary

This new Style rule is made as a part of adding support for NetBSD KNF in clang-format. This style Lines up BitField Declarations on consecutive lines with correct Indentation. The working of this Style rule shown below:

//Configuration
BitFieldDeclarationsOnePerLine: true

//Before Formatting:

unsigned int bas :3,  hh : 4, jjj : 8;


unsigned int baz:1,
                fuz:5, 
                zap:2;

//After Formatting:

unsigned int bas : 3,
             hh : 4,
             jjj : 8;

unsigned int baz : 1,
             fuz : 5,
             zap : 2;

This style is formatted even if the one-line declaration line is less than the column limit.

There is a minor Bug:
Comments after the bitfield in before the break line will not cause a proper indentation.

//Before Formatting:

unsigned int baz:1,     /* foo*/
             fuz:5,          /*bar*/
             zap:2;

//After Formatting:

unsigned int  baz : 1, /* foo*/
        fuz : 5,                 /*bar*/
        zap : 2;

Diff Detail

Repository
rC Clang

Event Timeline

Manikishan created this revision.Jun 9 2019, 12:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2019, 12:21 PM
Herald added a subscriber: krytarowski. · View Herald Transcript
lebedev.ri retitled this revision from Added New Style Rule: BitFieldDeclsOnSeparateLines to [clang-format] Added New Style Rule: BitFieldDeclsOnSeparateLines.Jun 9 2019, 12:28 PM
lebedev.ri edited reviewers, added: klimek, krasimir, Typz; removed: aaron.ballman, rsmith.
Manikishan edited the summary of this revision. (Show Details)
Manikishan edited the summary of this revision. (Show Details)Jun 9 2019, 12:34 PM
Manikishan edited the summary of this revision. (Show Details)
Manikishan edited the summary of this revision. (Show Details)
Manikishan edited the summary of this revision. (Show Details)Jun 9 2019, 12:36 PM
Manikishan edited the summary of this revision. (Show Details)
MyDeveloperDay requested changes to this revision.Jun 9 2019, 2:45 PM
MyDeveloperDay added a subscriber: MyDeveloperDay.

You need to add a unit test in clang/unittests

docs/ClangFormatStyleOptions.rst
194

Nit: Aligns Bitfield Declarations

I think Align is a better work than Linesup

196

align

197

end with :

lib/Format/TokenAnnotator.cpp
2920

elide the {}

This revision now requires changes to proceed.Jun 9 2019, 2:45 PM
Manikishan added a subscriber: mgorny.
mgorny requested changes to this revision.Jun 10 2019, 8:52 AM
mgorny added inline comments.
docs/ClangFormatStyleOptions.rst
194

'separate'

include/clang/Format/Format.h
104

Also update this to match docs.

lib/Format/ContinuationIndenter.cpp
281

Space after 'if' and before '{'. Also below.

lib/Format/TokenAnnotator.cpp
2921

Misindent.

This revision now requires changes to proceed.Jun 10 2019, 8:52 AM
Manikishan marked 4 inline comments as done.

Added unittests and made the changes suggested by @mgorny

Made some missing style modifications in the last revision

Manikishan marked 4 inline comments as done.

Updated unittest

MyDeveloperDay requested changes to this revision.Jun 10 2019, 12:49 PM
MyDeveloperDay added inline comments.
docs/ClangFormatStyleOptions.rst
193

Isn't the documentation normally alphabetric? shouldn't it be after AlignConsecutiveAssignments

include/clang/Format/Format.h
104

Align

112

I think this should be alphabetic in this file (not sure though check the rest of it)

are you happy with the name? BitFieldDeclsOnSeparateLines?

  1. people often spell Separate incorrectly (didn't you?), this could lead to misconfigured
  2. isn't this really a ''Break'' rule

I want to say this might better as something like "BreakAfterBitFieldDecl"

lib/Format/ContinuationIndenter.cpp
281

something here doesn't feel quite right,, without trying the code change myself I cannot tell, did you ever try this code without having the same clause in canBreak() and mustBreak()? (i.e. just put it in mustBreak)

The reason I ask is I'm unclear as to why the other mustBreak() rules aren't here in canBreak() if thats the case

lib/Format/TokenAnnotator.cpp
2921

This code appears 3 times (does it need to appear 3 times?), do we need some sort of

bool isBitField(FormatToken)
{
    ...
}

Should a bit field check for the existence of a number after the colon? I can't think of other C++ constructs that appear as

comma identifier colon

but given that clang-format is used for ObjC,ProtoBuf,Java,JavaScript,C# I'm pretty sure something odd is going to happen with JavaScript named parameters, to be honest I think this is going to cause the following to get reformatted

MyFunctionCall({ xPosition: 20, yPosition: 50, width: 100, height: 5, drawingNow: true });

MyFunctionCall({ xPosition: 20**, yPosition: 50,**
                            width: 100, 
                            height: 5, 
                            drawingNow: true });

or something like that

unittests/Format/FormatTest.cpp
3671

please add a test with comments (it will get logged)

unsigned int baz : 11, /*motor control flags*/
                     add: 2    /* control code for turning the lights on */ ,
                     foo: 3     /* (unused */
This revision now requires changes to proceed.Jun 10 2019, 12:49 PM
Manikishan marked 4 inline comments as done.

Changed Style name to OnePerLineBitFieldDecl

Manikishan marked an inline comment as done.

mark sure you mark off the comments as you consider them done.

lib/Format/FormatToken.h
519

this looks like it needs a clang-format as its not aligned with function that follows

527

couldn't you write this as just and lose the `if`

return (T->Tok.is(tok::comma) && Tok.is(tok::identifier) && T->Next->Tok.is(tok::colon));
Manikishan retitled this revision from [clang-format] Added New Style Rule: BitFieldDeclsOnSeparateLines to [clang-format] Added New Style Rule: OnePerLineBitFieldDecl.Jun 11 2019, 6:17 AM

Nit: if you going for "OnePerLine" in the name rather than "Break" can you keep it consistent with ConstructorInitializerAllOnOneLineOrOnePerLine and put the type first and then the operation i.e. BitFieldOnePerLine (this will help keep all BitField options together if there are ever more than one), I also think either don't use Decl at all or use the full word Declarations as in AlignConsecutiveDeclarations , AlwaysBreakTemplateDeclarations etc

Manikishan marked 4 inline comments as done and 2 inline comments as done.
Manikishan retitled this revision from [clang-format] Added New Style Rule: OnePerLineBitFieldDecl to [clang-format] Added New Style Rule: BitFieldDeclarationsOnePerLine.
MyDeveloperDay added inline comments.Jun 13 2019, 2:48 PM
lib/Format/FormatToken.h
524

do you think you might need

(T && T->Tok.is(tok::comma)......

lib/Format/TokenAnnotator.cpp
2920

can't this use Right.isBitField()

Manikishan edited the summary of this revision. (Show Details)Wed, Jun 26, 5:44 AM
unittests/Format/FormatTest.cpp
3671

any thoughts on these comments?

Manikishan marked 5 inline comments as done.Mon, Jul 15, 12:21 PM
Manikishan added inline comments.
unittests/Format/FormatTest.cpp
3671

Sorry, I totally have to keep this aside because of some reasons, will update within a day.

Manikishan marked an inline comment as done.Mon, Jul 15, 12:23 PM
Manikishan added inline comments.
lib/Format/TokenAnnotator.cpp
2921

Yes, you are correct this patch is failing many Javascript Tests. Will refactor the implementation.

Manikishan marked an inline comment as done.Thu, Jul 18, 9:19 AM
Manikishan added inline comments.
lib/Format/FormatToken.h
524

I have found a method FieldDecl::isBitField() which check whether a current field is a BitField, But FieldDecl is not used anywhere in lib/Format/ . I think it may not be related to Formatting and so I have to modify my declaration of isBitield() in FormatToken.h.
Can I get suggestions whether my understanding is correct?