Re: [Xen-devel] [PATCH] Add clang-format file for Xen Hypervisor format

[Lars Kurth <lars.kurth.xen@xxxxxxxxx>]

Hi Ishani,

> On 12 Apr 2017, at 11:49, Ishani Chugh <chugh.ishani@xxxxxxxxxxxxxxxxxxx> 
> wrote:

Here you would normally add a short description of the patch (also called cover 
letter). In this case, you probably want to describe:
a) What you have done
b) Link to 
https://wiki.xenproject.org/wiki/Outreach_Program_Projects#Code_Standards_Checking_using_clang-format,
 
https://docs.google.com/document/d/1tCcwZ9K38ToLGTPHZkfs2sS4s4YulrGoH8LIHwBMbg4/edit
 and 
https://docs.google.com/document/d/10NJn-QvO1TvyJJJGE2PD6FtElYCT3neBAffIqeWHdiE/edit
c) You can also link to your github repo
d) You may want to add the two command lines or some instructions showing how 
to use the patch

> Signed-off-by: Ishani Chugh <chugh.ishani@xxxxxxxxxxxxxxxxxxx>
> ---
> tools/clang-format/tests/.clang-format | 88 ++++++++++++++++++++++++++++++++++
> tools/clang-format/tests/correct.c     | 73 ++++++++++++++++++++++++++++
> tools/clang-format/tests/test.c        | 69 ++++++++++++++++++++++++++
> 3 files changed, 230 insertions(+)
> create mode 100644 tools/clang-format/tests/.clang-format
> create mode 100644 tools/clang-format/tests/correct.c
> create mode 100644 tools/clang-format/tests/test.c
> 
> diff --git a/tools/clang-format/tests/.clang-format 
> b/tools/clang-format/tests/.clang-format
> new file mode 100644
> index 0000000..2229910
> --- /dev/null
> +++ b/tools/clang-format/tests/.clang-format
> @@ -0,0 +1,88 @@
> +---
> +Language:        Cpp
> +# BasedOnStyle:  LLVM

This would be a good place to list in a comment, which coding style patterns 
the file covers. In other words, everything that has a yes in 
https://docs.google.com/document/d/1tCcwZ9K38ToLGTPHZkfs2sS4s4YulrGoH8LIHwBMbg4/edit

The other thing, which is a little unclear is whether the file applies to 
- http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE only
- http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/CODING_STYLE 
only
- or both

I think it is OK to just cover one file.

> +AccessModifierOffset: 0
> +AlignAfterOpenBracket: Align
> +AlignConsecutiveAssignments: false
> +AlignConsecutiveDeclarations: false
> +AlignEscapedNewlinesLeft: true
> +AlignOperands:   true
> +AlignTrailingComments: true
> +AllowAllParametersOfDeclarationOnNextLine: true
> +AllowShortBlocksOnASingleLine: false
> +AllowShortCaseLabelsOnASingleLine: false
> +AllowShortFunctionsOnASingleLine: All
> +AllowShortIfStatementsOnASingleLine: false
> +AllowShortLoopsOnASingleLine: false
> +AlwaysBreakAfterDefinitionReturnType: None
> +AlwaysBreakAfterReturnType: None
> +AlwaysBreakBeforeMultilineStrings: false
> +AlwaysBreakTemplateDeclarations: true
> +BinPackArguments: false
> +BinPackParameters: false
> +BraceWrapping:   
> +  AfterClass:      true
> +  AfterControlStatement: true
> +  AfterEnum:       true
> +  AfterFunction:   true
> +  AfterNamespace:  true
> +  AfterObjCDeclaration: true
> +  AfterStruct:     true
> +  AfterUnion:      true
> +  BeforeCatch:     true
> +  BeforeElse:      true
> +  IndentBraces:    false
> +BreakBeforeBinaryOperators: None
> +BreakBeforeBraces: Custom
> +BreakBeforeTernaryOperators: true
> +BreakConstructorInitializersBeforeComma: false
> +ColumnLimit:     80
> +CommentPragmas:  '^ IWYU pragma:'
> +ContinuationIndentWidth: 4
> +Cpp11BracedListStyle: true
> +DerivePointerAlignment: false
> +DisableFormat:   false
> +ExperimentalAutoDetectBinPacking: false
> +ForEachMacros:   [ foreach, Q_FOREACH, BOOST_FOREACH ]
> +IncludeCategories: 
> +  - Regex:           '^"(llvm|llvm-c|clang|clang-c)/'
> +    Priority:        2
> +  - Regex:           '^(<|"(gtest|isl|json)/)'
> +    Priority:        3
> +  - Regex:           '.*'
> +    Priority:        1
> +IndentCaseLabels: false
> +IndentWidth:     4
> +IndentWrappedFunctionNames: false
> +KeepEmptyLinesAtTheStartOfBlocks: false
> +MacroBlockBegin: ''
> +MacroBlockEnd:   ''
> +MaxEmptyLinesToKeep: 1
> +NamespaceIndentation: None
> +ObjCBlockIndentWidth: 2
> +ObjCSpaceAfterProperty: false
> +ObjCSpaceBeforeProtocolList: true
> +PenaltyBreakBeforeFirstCallParameter: 19
> +PenaltyBreakComment: 300
> +PenaltyBreakFirstLessLess: 120
> +PenaltyBreakString: 1000
> +PenaltyExcessCharacter: 1000000
> +PenaltyReturnTypeOnItsOwnLine: 60
> +PointerAlignment: Left
> +ReflowComments:  true
> +SortIncludes:    false
> +SpaceAfterCStyleCast: false
> +SpaceBeforeAssignmentOperators: true
> +SpaceBeforeParens: ControlStatements
> +SpaceInEmptyParentheses: false
> +SpacesBeforeTrailingComments: 1
> +SpacesInAngles:  false
> +SpacesInContainerLiterals: true
> +SpacesInCStyleCastParentheses: true
> +SpacesInParentheses: true
> +SpacesInSquareBrackets: false
> +Standard:        Cpp11
> +TabWidth:        4
> +UseTab:          Never
> +...
> +
> diff --git a/tools/clang-format/tests/correct.c 
> b/tools/clang-format/tests/correct.c
> new file mode 100644
> index 0000000..882d50a
> --- /dev/null
> +++ b/tools/clang-format/tests/correct.c
> @@ -0,0 +1,73 @@

I think it would be extremely useful, to add a reference in a comment to the 
coding standard document for each tested code-snippet. 
I am going to add an example under main()

> +/*
> +This is used to check if includes are sorted or not
> +*/
> +#include <unistd.h>
> +#include <stdio.h>
> +#define true 1
> +
> +struct sample
> +{
> +    int a;
> +};
> +
> +/*
> +This is used to check support for small function in a line
> +*/
> +int a( int x, int y ) {}
> +
> +int main()
> +{
> +    // This will test indentation

Coming back to my earlier point, this tests 
http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE section 
Indentation (lines 17-32)
So you could say

      // This tests Indentation
      // See xen.git CODING_STYLE section Indentation (lines 17-32)

> +
> +    if ( true )
> +    {
> +        if ( true )
> +        {
> +            printf( "this should be indented\n" );
> +            if ( true )
> +            {
> +                printf( "this should be indented too\n" );
> +            }
> +        }
> +    }
> +
> +    /*
> +    This will check whitespaces format
> +    */
> +
> +    if ( ( 1 & 2 ) == 42 )
> +        printf( "dfadsf\n" );
> +
> +    /*
> +    This will test that whitespaces are not added for . and -> and the left 
> position of *
> +    */
> +    struct sample ob;
> +    struct sample* ob1;
> +    ob.a = 10;
> +    ob1->a = 10;
> +    int x;
> +    int y;
> +    char* p;
> +
> +    /*
> +    This will break the line. It also checks addition of spaces in binary 
> operators
> +    */
> +    if ( 1 > 2 && 1 > 2 && 1 > 2 && 1 > 2 && 1 > 2 && 1 > 2 && 1 > 2 && 1 > 
> 2 &&
> +         1 > 2 && 1 > 2 && 1 > 2 && 1 > 2 && 1 > 2 && 1 > 2 && 1 > 2 && 1 > 
> 2 )
> +        p = 
> "adsaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
> +            "aaaaaaaaaaaaaaaaaaaaa";
> +    printf( 
> "adddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd"
> +            "dddddddddddddddddadad" );
> +
> +    for ( ;; )
> +    {
> +        int z = 5;
> +        break;
> +    }
> +    /*
> +    Check for template break in template
> +    */
> +    template <class P>
> +    void updateMenuList( P*, struct menu* );
> +    return 0;
> +}
> \ No newline at end of file
> diff --git a/tools/clang-format/tests/test.c b/tools/clang-format/tests/test.c
> new file mode 100644
> index 0000000..2e7eaf2
> --- /dev/null
> +++ b/tools/clang-format/tests/test.c

How is this file different from correct.c? 
It looks to me that test.c is a wrongly formatted version of correct.c and when 
you pass it into clang-format it is re-formatted to match correct.c

If so, you want to include this information in the cover letter (see above). Or 
you could write little bash script which shows how this works

> @@ -0,0 +1,69 @@
> +/*
> +This is used to check if includes are sorted or not
> +*/
> +#include<unistd.h>
> +#include<stdio.h>
> +#define true 1
> +
> +struct sample{
> +     int a;
> +};
> +
> +/*
> +This is used to check support for small function in a line
> +*/
> +int a(int x, int y)
> +{}
> +
> +int main()
> +{
> +     //This will test indentation
> +
> +if(true)
> +{
> +if(true)
> +{
> +printf("this should be indented\n");
> +if(true)
> +{
> +printf("this should be indented too\n");
> +                     }
> +}
> +}
> +
> +/*
> +This will check whitespaces format
> +*/
> +
> +if((1&2)==42)
> +printf("dfadsf\n" );
> +
> +/*
> +This will test that whitespaces are not added for . and -> and the left 
> position of *
> +*/
> + struct sample ob;
> + struct sample* ob1;
> +ob.a = 10;
> +ob1->a =10;
> +     int x;
> +     int y;
> +     char *p;
> +
> +/*
> +This will break the line. It also checks addition of spaces in binary 
> operators
> +*/
> +     if (1>2 && 1>2 && 1>2 && 1>2 && 1>2 && 1>2 && 1>2 && 1>2 && 1>2 && 1>2 
> && 1>2 && 1>2 && 1>2 && 1>2 && 1>2 && 1>2)
> +     p = 
> "adsaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
> +     
> printf("addddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddadad");
> +
> +     for(;;)
> +             {
> +             int z = 5;
> +break;
> +}
> +/*
> +Check for template break in template
> +*/
> +template <class P>   void updateMenuList(P*, struct menu*);
> +     return 0;
> +}
> \ No newline at end of file
> -- 
> 2.7.4
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel