[Xen-devel,2/2] CODING_STYLE: Forbid nested block in the hypervisor code

Message ID 20170704121214.32145-2-julien.grall@arm.com
State New
Headers show
Series
  • [Xen-devel,1/2] CODING_STYLE: removing trailing whitespaces
Related show

Commit Message

Julien Grall July 4, 2017, 12:12 p.m.
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(+)

Comments

Jan Beulich July 4, 2017, 12:20 p.m. | #1
>>> 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
Andrew Cooper July 4, 2017, 2:17 p.m. | #2
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
Julien Grall July 4, 2017, 5:16 p.m. | #3
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,

Patch

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