[PATCH 1/2] CODING_STYLE: removing trailing whitespaces

[PATCH 1/2] CODING_STYLE: removing trailing whitespaces

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 CODING_STYLE | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/CODING_STYLE b/CODING_STYLE
index 4c3b53a754..6cc5b774cf 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -4,11 +4,11 @@ Coding Style for the Xen Hypervisor
 The Xen coding style described below is the coding style used by the
 Xen hypervisor itself (xen/*) as well as various associated low-level
 libraries (e.g. tools/libxc/*).
-        
+
 An exception is made for files which are imported from an external
 source. In these cases the prevailing coding style of the upstream
 source is generally used (commonly the Linux coding style).
-        
+
 Other parts of the code base may use other coding styles, sometimes
 explicitly (e.g. tools/libxl/CODING_STYLE) but often implicitly (Linux
 coding style is fairly common). In general you should copy the style
-- 
2.11.0


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

[PATCH 2/2] CODING_STYLE: Forbid nested block in the hypervisor code

[Julien Grall]

When removing if/for/while statements, the code should be reworked to
remove the { } and the extra indentation. This is improving code
maintainability and code readability.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    This patch was triggered whilst reviewing a patch [1] on ARM that
    remove the if statement but keep the braces around. I personally
    dislike such changes as it make the code less and readable maintenable
    in the future. Stefano asked to send a patch against CODING_STYLE to
    apply the rule to all the hypervisor code.

    I am not entirely sure about the name of those type of block and the
    wording. I would appreciate any advice here.

    [1] https://lists.xen.org/archives/html/xen-devel/2017-07/msg00060.html
---
 CODING_STYLE | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/CODING_STYLE b/CODING_STYLE
index 6cc5b774cf..d1575a7068 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -88,6 +88,21 @@ Braces should be omitted for blocks with a single statement. e.g.,
 if ( condition )
     single_statement();
 
+Nested blocks
+-------------
+
+Nested blocks should be avoided e.g:
+
+int a;
+{
+    int b;
+    /* Do stuff */
+}
+/* Do stuff */
+
+More importantly, if a patch requires to remove an if/while/for statements, the
+code should be reworked rather than introducing a nested block.
+
 Comments
 --------
 
-- 
2.11.0


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

Re: [PATCH 2/2] CODING_STYLE: Forbid nested block in the hypervisor code

[Jan Beulich]

>>> On 04.07.17 at 14:12, <julien.grall@arm.com> wrote:
> When removing if/for/while statements, the code should be reworked to
> remove the { } and the extra indentation.

Yes.

> This is improving code maintainability and code readability.

For the given example, yes. However, there are (rare) cases where
having such nested blocks actually improves readability, for example
in certain combinations with preprocessor conditionals. Hence I don't
think we should forbid them.

Jan


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

Re: [PATCH 1/2] CODING_STYLE: removing trailing whitespaces

[Wei Liu]

On Tue, Jul 04, 2017 at 01:12:13PM +0100, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Applied this patch.

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

Re: [PATCH 2/2] CODING_STYLE: Forbid nested block in the hypervisor code

[Andrew Cooper]

On 04/07/17 13:20, Jan Beulich wrote:
>>>> On 04.07.17 at 14:12, <julien.grall@arm.com> wrote:
>> When removing if/for/while statements, the code should be reworked to
>> remove the { } and the extra indentation.
> Yes.
>
>> This is improving code maintainability and code readability.
> For the given example, yes. However, there are (rare) cases where
> having such nested blocks actually improves readability, for example
> in certain combinations with preprocessor conditionals. Hence I don't
> think we should forbid them.

There are also a few specific cases where it is useful to use blocks
like that to introduce a new variable, where introducing it at function
level scope isn't appropriate.  (Alternatively, we could switch from C89
to C99, but that is a separate discussion).

I agree that we should discourage the use of blocks like this, but not
forbid them outright.

~Andrew

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

Re: [PATCH 2/2] CODING_STYLE: Forbid nested block in the hypervisor code

[Julien Grall]

Hi,

On 07/04/2017 03:17 PM, Andrew Cooper wrote:
> On 04/07/17 13:20, Jan Beulich wrote:
>>>>> On 04.07.17 at 14:12, <julien.grall@arm.com> wrote:
>>> When removing if/for/while statements, the code should be reworked to
>>> remove the { } and the extra indentation.
>> Yes.
>>
>>> This is improving code maintainability and code readability.
>> For the given example, yes. However, there are (rare) cases where
>> having such nested blocks actually improves readability, for example
>> in certain combinations with preprocessor conditionals. Hence I don't
>> think we should forbid them.
> 
> There are also a few specific cases where it is useful to use blocks
> like that to introduce a new variable, where introducing it at function
> level scope isn't appropriate.  (Alternatively, we could switch from C89
> to C99, but that is a separate discussion).
> 
> I agree that we should discourage the use of blocks like this, but not
> forbid them outright.

Thank you both for the feedback. I will rework the proposal to 
discourage contributor rather than forbid.

Cheers,

-- 
Julien Grall

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