| Message ID | 20170704121214.32145-2-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Xen-devel,1/2] CODING_STYLE: removing trailing whitespaces | expand |
>>> 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
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
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,
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 --------
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(+)