This is an archive of the discontinued LLVM Phabricator instance.

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

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
195

Nit: Aligns Bitfield Declarations

I think Align is a better work than Linesup

197

align

198

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
195

'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
194

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
285

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 ↗(On Diff #203860)

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 ↗(On Diff #204040)

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

527 ↗(On Diff #204040)

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 ↗(On Diff #204340)

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)Jun 26 2019, 5:44 AM
unittests/Format/FormatTest.cpp
3671 ↗(On Diff #203860)

any thoughts on these comments?

Manikishan marked 5 inline comments as done.Jul 15 2019, 12:21 PM
Manikishan added inline comments.
unittests/Format/FormatTest.cpp
3671 ↗(On Diff #203860)

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

Manikishan marked an inline comment as done.Jul 15 2019, 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.Jul 18 2019, 9:19 AM
Manikishan added inline comments.
lib/Format/FormatToken.h
524 ↗(On Diff #204340)

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?

MyDeveloperDay added a project: Restricted Project.Sep 25 2019, 1:47 AM
MyDeveloperDay added inline comments.Sep 26 2019, 1:30 AM
lib/Format/FormatToken.h
524 ↗(On Diff #204340)

FieldDecl will be part of the AST tree, but clang-format doesn't use that (or it would need compiler arguments etc..)

Take a look at TokenAnnotator.cpp... (and try running clang-format with the -debug option), it may show you that the bitfield can be identified by the TT_BitFieldColon type rather than by a colon directly.

} else if (CurrentToken && CurrentToken->is(tok::numeric_constant)) {
  Tok->Type = TT_BitFieldColon;
Manikishan marked an inline comment as done.Sep 26 2019, 1:35 AM
Manikishan added inline comments.
lib/Format/FormatToken.h
524 ↗(On Diff #204340)

Sure will have a look at it, and figure out how can I check for bitfields

MyDeveloperDay requested changes to this revision.Dec 4 2019, 12:54 AM
MyDeveloperDay added inline comments.
lib/Format/Format.cpp
455

Alphabetical

This revision now requires changes to proceed.Dec 4 2019, 12:54 AM