[PATCH edk2-CCSS 0/3] Coding Standards: add rule for documenting spurious variable assignments

[PATCH edk2-CCSS 0/3] Coding Standards: add rule for documenting spurious variable assignments

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2-CCodingStandardsSpecification.git
Branch: spurious_assign_bz_607

HTML-rendered views of the modified pages:
- https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607
- https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/62_comments.html
- https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/64_what_you_must_comment.html

The first two patches are cleanups for things that popped up in the
discussion in <https://bugzilla.tianocore.org/show_bug.cgi?id=607>.

The third patch is the one fixing the BZ.

Thanks,
Laszlo

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Rebecca Cran <rebecca@bsdio.com>

Laszlo Ersek (3):
  comments: remove "Horror Vacui" rule
  comments: restrict and clarify applicability of "/*" comments
  must comment: add rule for documenting spurious variable assignments

 6_documenting_software/62_comments.md              | 20 +---------
 6_documenting_software/64_what_you_must_comment.md | 39 ++++++++++++++++++++
 README.md                                          |  1 +
 3 files changed, 42 insertions(+), 18 deletions(-)

-- 
2.19.1.3.g30247aa5d201

[PATCH edk2-CCSS 1/3] comments: remove "Horror Vacui" rule

[Laszlo Ersek]

The rule about avoiding comments in C code where the opening comment
characters are alone on a line conflicts with everyday edk2 practice --
we use comments like

  //
  // If the total number of lines in the popup is zero, then ASSERT()
  //

all the time. This comment style is actively enforced by reviewers and
maintainers. Remove the rule in order for the CCS to match reality.

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=607
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 6_documenting_software/62_comments.md | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/6_documenting_software/62_comments.md b/6_documenting_software/62_comments.md
index 208ce346f607..ae04e008a1eb 100644
--- a/6_documenting_software/62_comments.md
+++ b/6_documenting_software/62_comments.md
@@ -66,23 +66,7 @@ not behave the same.
 Banners can be useful to highlight logical divisions within a file; such as
 before vital sections. This type of usage should be minimized.
 
-### 6.2.3 Avoid comments where the opening comment characters are alone on a line.
-
-```
-/*
- * VIOLATION: Horror Vacui
-*/
-```
-
-or
-
-```c
-//
-// VIOLATION: Horror Vacui
-//
-```
-
-### 6.2.4 Do not include jokes or obtuse references in comments.
+### 6.2.3 Do not include jokes or obtuse references in comments.
 
 ```
 "Out of cheese error! Redo from start."
-- 
2.19.1.3.g30247aa5d201

[PATCH edk2-CCSS 2/3] comments: restrict and clarify applicability of "/*" comments

[Laszlo Ersek]

Section "6.2 Comments" seems to permit "/*" for multi-line comments in
general. That's incorrect; "/*" comments are permitted only for a subset
of multi-line comments (namely Doxygen-style file and function header
comment blocks). Update the rule accordingly.

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=607
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 6_documenting_software/62_comments.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/6_documenting_software/62_comments.md b/6_documenting_software/62_comments.md
index ae04e008a1eb..5feb5cee2055 100644
--- a/6_documenting_software/62_comments.md
+++ b/6_documenting_software/62_comments.md
@@ -54,7 +54,7 @@ minimal familiarity with the code. Clarity is important, but one should also
 strive for terse and concise comments. One should be able to see both the
 comment, and the code being commented on, on the same screen.
 
-### 6.2.1 Only use C style, "/*", comments for multi-line comments and on the same line as pre-processor directives.
+### 6.2.1 Only use C style, "/*", comments on the same line as pre-processor directives, and in Doxygen-style file and function header comment blocks.
 
 Compile can vary in their support for use of `//` in preprocessor directives
 (e.g. `#define`). Note that the mixing of `/* ... */` and `//` is not handled
-- 
2.19.1.3.g30247aa5d201

[PATCH edk2-CCSS 3/3] must comment: add rule for documenting spurious variable assignments

[Laszlo Ersek]

Problem statement from Ard:

> Sometimes, the GCC compiler warns about variables potentially being used
> without having been initialized, while visual inspection reveals that
> this is impossible. In such cases, we need to initialize such a variable
> to an arbitrary value only to avoid breaking the build, given our policy
> to treat warnings as errors.

In such cases we generally use

  LocalIntegerVariable = 0;

and

  LocalPointerVariable = NULL;

which takes care of the incorrect warning. However, it also makes the
human analysis of any subsequent logic harder, because it suggests that
assigning that specific zero or NULL value to the local variable is
*required* by the subsequent logic.

In order to highlight such assignments, whose sole purpose is to suppress
invalid "use before init" warnings from compilers or static analysis
tools, we should mandate comments such as:

  //
  // set LocalVariable to suppress incorrect compiler/analyzer warnings
  //
  LocalVariable = 0;

(Magic values such as 0xDEADBEEF, which would obviate the necessity of
explicit comments, have been considered, and rejected for stylistic
reasons.)

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=607
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 6_documenting_software/64_what_you_must_comment.md | 39 ++++++++++++++++++++
 README.md                                          |  1 +
 2 files changed, 40 insertions(+)

diff --git a/6_documenting_software/64_what_you_must_comment.md b/6_documenting_software/64_what_you_must_comment.md
index abb2114bf5bc..9e51c2e45816 100644
--- a/6_documenting_software/64_what_you_must_comment.md
+++ b/6_documenting_software/64_what_you_must_comment.md
@@ -58,3 +58,42 @@ instance differs.
 
 When possible, you should also list the requirements that are satisfied by the
 code.
+
+### 6.4.6 Comment spurious variable assignments.
+
+A compiler or static code analyzer may warn that an object with automatic or
+allocated storage duration is read without having been initialized, while
+visual inspection reveals that this is impossible.
+
+In order to suppress such a warning (which is emitted due to invalid data flow
+analysis), developers explicitly assign the affected object the value to which
+the same object would be initialized automatically, had the object static
+storage duration, and no initializer. (The value assigned could be arbitrary;
+the above-mentioned value is chosen for stylistic reasons.) For example:
+
+```c
+UINTN LocalIntegerVariable;
+VOID  *LocalPointerVariable;
+
+LocalIntegerVariable = 0;
+LocalPointerVariable = NULL;
+```
+
+This kind of assignment is difficult to distinguish from assignments where the
+initial value of an object is meaningful, and is consumed by other code without
+an intervening assignment. Therefore, each such assignment must be documented,
+as follows:
+
+```c
+UINTN LocalIntegerVariable;
+VOID  *LocalPointerVariable;
+
+//
+// set LocalIntegerVariable to suppress incorrect compiler/analyzer warnings
+//
+LocalIntegerVariable = 0;
+//
+// set LocalPointerVariable to suppress incorrect compiler/analyzer warnings
+//
+LocalPointerVariable = NULL;
+```
diff --git a/README.md b/README.md
index e26133540368..0648819f0d3a 100644
--- a/README.md
+++ b/README.md
@@ -113,3 +113,4 @@ Copyright (c) 2006-2017, Intel Corporation. All rights reserved.
 | 2.2      | Convert to Gitbook                                                                                                                                | June 2017  |
 |          | [#425](https://bugzilla.tianocore.org/show_bug.cgi?id=425) [CCS] clarify line breaking and indentation requirements for multi-line function calls |            |
 |          | [#1656](https://bugzilla.tianocore.org/show_bug.cgi?id=1656) Update all Wiki pages for the BSD+Patent license change with SPDX identifiers        |            |
+|          | [#607](https://bugzilla.tianocore.org/show_bug.cgi?id=607) Document code comment requirements for spurious variable assignments                   |            |
-- 
2.19.1.3.g30247aa5d201

Re: [edk2-devel] [PATCH edk2-CCSS 2/3] comments: restrict and clarify applicability of "/*" comments

[Philippe Mathieu-Daudé]

On 9/5/19 8:38 PM, Laszlo Ersek wrote:
> Section "6.2 Comments" seems to permit "/*" for multi-line comments in
> general. That's incorrect; "/*" comments are permitted only for a subset
> of multi-line comments (namely Doxygen-style file and function header
> comment blocks). Update the rule accordingly.
> 
> Cc: Andrew Fish <afish@apple.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=607
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  6_documenting_software/62_comments.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/6_documenting_software/62_comments.md b/6_documenting_software/62_comments.md
> index ae04e008a1eb..5feb5cee2055 100644
> --- a/6_documenting_software/62_comments.md
> +++ b/6_documenting_software/62_comments.md
> @@ -54,7 +54,7 @@ minimal familiarity with the code. Clarity is important, but one should also
>  strive for terse and concise comments. One should be able to see both the
>  comment, and the code being commented on, on the same screen.
>  
> -### 6.2.1 Only use C style, "/*", comments for multi-line comments and on the same line as pre-processor directives.
> +### 6.2.1 Only use C style, "/*", comments on the same line as pre-processor directives, and in Doxygen-style file and function header comment blocks.
>  
>  Compile can vary in their support for use of `//` in preprocessor directives
>  (e.g. `#define`). Note that the mixing of `/* ... */` and `//` is not handled
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [PATCH edk2-CCSS 3/3] must comment: add rule for documenting spurious variable assignments

[Philippe Mathieu-Daudé]

Hi Laszlo,

(Cc'ing Ard)

On 9/5/19 8:38 PM, Laszlo Ersek wrote:
> Problem statement from Ard:
> 
>> Sometimes, the GCC compiler warns about variables potentially being used
>> without having been initialized, while visual inspection reveals that
>> this is impossible. In such cases, we need to initialize such a variable
>> to an arbitrary value only to avoid breaking the build, given our policy
>> to treat warnings as errors.

This is annoying.

I suppose using CFLAGS+='-Wno-uninitialized -Wmaybe-uninitialized' is
not an acceptable option.

> 
> In such cases we generally use
> 
>   LocalIntegerVariable = 0;
> 
> and
> 
>   LocalPointerVariable = NULL;
> 
> which takes care of the incorrect warning. However, it also makes the
> human analysis of any subsequent logic harder, because it suggests that
> assigning that specific zero or NULL value to the local variable is
> *required* by the subsequent logic.

What about having explicit definitions to silent warnings, so we don't
need to add comments?

#define UNINITIALIZED_INTEGER 0
#define UNINITIALIZED_POINTER NULL

Human review becomes trivial:

   LocalPointerVariable = UNINITIALIZED_POINTER;

> In order to highlight such assignments, whose sole purpose is to suppress
> invalid "use before init" warnings from compilers or static analysis
> tools, we should mandate comments such as:
> 
>   //
>   // set LocalVariable to suppress incorrect compiler/analyzer warnings
>   //
>   LocalVariable = 0;
> 
> (Magic values such as 0xDEADBEEF, which would obviate the necessity of
> explicit comments, have been considered, and rejected for stylistic
> reasons.)
> 
> Cc: Andrew Fish <afish@apple.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=607
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  6_documenting_software/64_what_you_must_comment.md | 39 ++++++++++++++++++++
>  README.md                                          |  1 +
>  2 files changed, 40 insertions(+)
> 
> diff --git a/6_documenting_software/64_what_you_must_comment.md b/6_documenting_software/64_what_you_must_comment.md
> index abb2114bf5bc..9e51c2e45816 100644
> --- a/6_documenting_software/64_what_you_must_comment.md
> +++ b/6_documenting_software/64_what_you_must_comment.md
> @@ -58,3 +58,42 @@ instance differs.
>  
>  When possible, you should also list the requirements that are satisfied by the
>  code.
> +
> +### 6.4.6 Comment spurious variable assignments.
> +
> +A compiler or static code analyzer may warn that an object with automatic or
> +allocated storage duration is read without having been initialized, while
> +visual inspection reveals that this is impossible.
> +
> +In order to suppress such a warning (which is emitted due to invalid data flow
> +analysis), developers explicitly assign the affected object the value to which
> +the same object would be initialized automatically, had the object static
> +storage duration, and no initializer. (The value assigned could be arbitrary;
> +the above-mentioned value is chosen for stylistic reasons.) For example:
> +
> +```c
> +UINTN LocalIntegerVariable;
> +VOID  *LocalPointerVariable;
> +
> +LocalIntegerVariable = 0;
> +LocalPointerVariable = NULL;
> +```
> +
> +This kind of assignment is difficult to distinguish from assignments where the
> +initial value of an object is meaningful, and is consumed by other code without
> +an intervening assignment. Therefore, each such assignment must be documented,
> +as follows:
> +
> +```c
> +UINTN LocalIntegerVariable;
> +VOID  *LocalPointerVariable;
> +
> +//
> +// set LocalIntegerVariable to suppress incorrect compiler/analyzer warnings
> +//
> +LocalIntegerVariable = 0;
> +//
> +// set LocalPointerVariable to suppress incorrect compiler/analyzer warnings
> +//
> +LocalPointerVariable = NULL;
> +```
> diff --git a/README.md b/README.md
> index e26133540368..0648819f0d3a 100644
> --- a/README.md
> +++ b/README.md
> @@ -113,3 +113,4 @@ Copyright (c) 2006-2017, Intel Corporation. All rights reserved.
>  | 2.2      | Convert to Gitbook                                                                                                                                | June 2017  |
>  |          | [#425](https://bugzilla.tianocore.org/show_bug.cgi?id=425) [CCS] clarify line breaking and indentation requirements for multi-line function calls |            |
>  |          | [#1656](https://bugzilla.tianocore.org/show_bug.cgi?id=1656) Update all Wiki pages for the BSD+Patent license change with SPDX identifiers        |            |
> +|          | [#607](https://bugzilla.tianocore.org/show_bug.cgi?id=607) Document code comment requirements for spurious variable assignments                   |            |
> 

Re: [PATCH edk2-CCSS 0/3] Coding Standards: add rule for documenting spurious variable assignments

[Leif Lindholm]

On Thu, Sep 05, 2019 at 08:38:17PM +0200, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2-CCodingStandardsSpecification.git
> Branch: spurious_assign_bz_607
> 
> HTML-rendered views of the modified pages:
> - https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607
> - https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/62_comments.html
> - https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/64_what_you_must_comment.html
> 
> The first two patches are cleanups for things that popped up in the
> discussion in <https://bugzilla.tianocore.org/show_bug.cgi?id=607>.
> 
> The third patch is the one fixing the BZ.

For 1 and 2,
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

For 3, I see no issue with it, but I do feel tempted by Phil's input
of using explicit macros (obviating the need for specific comment).
I seem to recall back in the mists of time we considered something
similar. Vaguely. Am I misremembering, or did we disount that option?

Regards,

Leif

> Thanks,
> Laszlo
> 
> Cc: Andrew Fish <afish@apple.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> 
> Laszlo Ersek (3):
>   comments: remove "Horror Vacui" rule
>   comments: restrict and clarify applicability of "/*" comments
>   must comment: add rule for documenting spurious variable assignments
> 
>  6_documenting_software/62_comments.md              | 20 +---------
>  6_documenting_software/64_what_you_must_comment.md | 39 ++++++++++++++++++++
>  README.md                                          |  1 +
>  3 files changed, 42 insertions(+), 18 deletions(-)
> 
> -- 
> 2.19.1.3.g30247aa5d201
> 

Re: [edk2-devel] [PATCH edk2-CCSS 3/3] must comment: add rule for documenting spurious variable assignments

[Laszlo Ersek]

On 09/06/19 10:13, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
> 
> (Cc'ing Ard)
> 
> On 9/5/19 8:38 PM, Laszlo Ersek wrote:
>> Problem statement from Ard:
>>
>>> Sometimes, the GCC compiler warns about variables potentially being used
>>> without having been initialized, while visual inspection reveals that
>>> this is impossible. In such cases, we need to initialize such a variable
>>> to an arbitrary value only to avoid breaking the build, given our policy
>>> to treat warnings as errors.
> 
> This is annoying.
> 
> I suppose using CFLAGS+='-Wno-uninitialized -Wmaybe-uninitialized' is
> not an acceptable option.

I don't have links handy, but around or before the time I filed
TianoCore#607, we had gone through all the possibilities. The issue may
have been possible to suppress with cmdline options for a particular
toolchain version, but I'm fairly sure it was impossible to solve for
all the toolchains simultaneously that edk2 supported at the time.

>>
>> In such cases we generally use
>>
>>   LocalIntegerVariable = 0;
>>
>> and
>>
>>   LocalPointerVariable = NULL;
>>
>> which takes care of the incorrect warning. However, it also makes the
>> human analysis of any subsequent logic harder, because it suggests that
>> assigning that specific zero or NULL value to the local variable is
>> *required* by the subsequent logic.
> 
> What about having explicit definitions to silent warnings, so we don't
> need to add comments?
> 
> #define UNINITIALIZED_INTEGER 0
> #define UNINITIALIZED_POINTER NULL
> 
> Human review becomes trivial:
> 
>    LocalPointerVariable = UNINITIALIZED_POINTER;

We did consider macros too, if I remember correctly. It was not liked.
(We definitely considered magic values, see 0xDEADBEEF below, and those
were clearly rejected.) People really seemed to want zero / NULL values,
open-coded. I disagreed, but accepted. The explicit comment suggestion
was a compromise from my side, therefore.

In this patch set, I wouldn't like to introduce a rule that is not based
in current practice. The code base is already full of the above kind of
zero / NULL assignment; the only coding style detail, from the rule
being suggested, is the comment.

While TianoCore#607 has been open, I've consistently directed developers
to it, for the proposed syntax. Therefore, if you look at the code base
today, you will find a large amount of the original un-annotated zero /
NULL assignment (where you can't immediately tell whether they are
algorithmically necessery or not), and a few instances of the wording
proposed here.

$ git grep 'incorrect compiler/analyzer'

In that regard, this patch set aims to codify existing practice -- I
just want to make the pattern more consistent.

Thanks
Laszlo

> 
>> In order to highlight such assignments, whose sole purpose is to suppress
>> invalid "use before init" warnings from compilers or static analysis
>> tools, we should mandate comments such as:
>>
>>   //
>>   // set LocalVariable to suppress incorrect compiler/analyzer warnings
>>   //
>>   LocalVariable = 0;
>>
>> (Magic values such as 0xDEADBEEF, which would obviate the necessity of
>> explicit comments, have been considered, and rejected for stylistic
>> reasons.)
>>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Rebecca Cran <rebecca@bsdio.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=607
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  6_documenting_software/64_what_you_must_comment.md | 39 ++++++++++++++++++++
>>  README.md                                          |  1 +
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/6_documenting_software/64_what_you_must_comment.md b/6_documenting_software/64_what_you_must_comment.md
>> index abb2114bf5bc..9e51c2e45816 100644
>> --- a/6_documenting_software/64_what_you_must_comment.md
>> +++ b/6_documenting_software/64_what_you_must_comment.md
>> @@ -58,3 +58,42 @@ instance differs.
>>  
>>  When possible, you should also list the requirements that are satisfied by the
>>  code.
>> +
>> +### 6.4.6 Comment spurious variable assignments.
>> +
>> +A compiler or static code analyzer may warn that an object with automatic or
>> +allocated storage duration is read without having been initialized, while
>> +visual inspection reveals that this is impossible.
>> +
>> +In order to suppress such a warning (which is emitted due to invalid data flow
>> +analysis), developers explicitly assign the affected object the value to which
>> +the same object would be initialized automatically, had the object static
>> +storage duration, and no initializer. (The value assigned could be arbitrary;
>> +the above-mentioned value is chosen for stylistic reasons.) For example:
>> +
>> +```c
>> +UINTN LocalIntegerVariable;
>> +VOID  *LocalPointerVariable;
>> +
>> +LocalIntegerVariable = 0;
>> +LocalPointerVariable = NULL;
>> +```
>> +
>> +This kind of assignment is difficult to distinguish from assignments where the
>> +initial value of an object is meaningful, and is consumed by other code without
>> +an intervening assignment. Therefore, each such assignment must be documented,
>> +as follows:
>> +
>> +```c
>> +UINTN LocalIntegerVariable;
>> +VOID  *LocalPointerVariable;
>> +
>> +//
>> +// set LocalIntegerVariable to suppress incorrect compiler/analyzer warnings
>> +//
>> +LocalIntegerVariable = 0;
>> +//
>> +// set LocalPointerVariable to suppress incorrect compiler/analyzer warnings
>> +//
>> +LocalPointerVariable = NULL;
>> +```
>> diff --git a/README.md b/README.md
>> index e26133540368..0648819f0d3a 100644
>> --- a/README.md
>> +++ b/README.md
>> @@ -113,3 +113,4 @@ Copyright (c) 2006-2017, Intel Corporation. All rights reserved.
>>  | 2.2      | Convert to Gitbook                                                                                                                                | June 2017  |
>>  |          | [#425](https://bugzilla.tianocore.org/show_bug.cgi?id=425) [CCS] clarify line breaking and indentation requirements for multi-line function calls |            |
>>  |          | [#1656](https://bugzilla.tianocore.org/show_bug.cgi?id=1656) Update all Wiki pages for the BSD+Patent license change with SPDX identifiers        |            |
>> +|          | [#607](https://bugzilla.tianocore.org/show_bug.cgi?id=607) Document code comment requirements for spurious variable assignments                   |            |
>>

Re: [PATCH edk2-CCSS 0/3] Coding Standards: add rule for documenting spurious variable assignments

[Laszlo Ersek]

On 09/06/19 14:26, Leif Lindholm wrote:
> On Thu, Sep 05, 2019 at 08:38:17PM +0200, Laszlo Ersek wrote:
>> Repo:   https://github.com/lersek/edk2-CCodingStandardsSpecification.git
>> Branch: spurious_assign_bz_607
>>
>> HTML-rendered views of the modified pages:
>> - https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607
>> - https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/62_comments.html
>> - https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/64_what_you_must_comment.html
>>
>> The first two patches are cleanups for things that popped up in the
>> discussion in <https://bugzilla.tianocore.org/show_bug.cgi?id=607>.
>>
>> The third patch is the one fixing the BZ.
> 
> For 1 and 2,
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
> For 3, I see no issue with it, but I do feel tempted by Phil's input
> of using explicit macros (obviating the need for specific comment).
> I seem to recall back in the mists of time we considered something
> similar.

Yes, I remember similarly.

> Vaguely. Am I misremembering, or did we disount that option?

Phil's current recommendation is what I would have preferred back then,
but it was rejected, as far as I recall. If I remember correctly, most
developers preferred naked NULLs / zeroes. I insisted on the comment as
a fallback / compromise, so that we'd have at least some visual cue.

I could be mis-remembering; we can restart that discussion if now the
macros are preferred.

Thanks,
Laszlo

> 
> Regards,
> 
> Leif
> 
>> Thanks,
>> Laszlo
>>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Rebecca Cran <rebecca@bsdio.com>
>>
>> Laszlo Ersek (3):
>>   comments: remove "Horror Vacui" rule
>>   comments: restrict and clarify applicability of "/*" comments
>>   must comment: add rule for documenting spurious variable assignments
>>
>>  6_documenting_software/62_comments.md              | 20 +---------
>>  6_documenting_software/64_what_you_must_comment.md | 39 ++++++++++++++++++++
>>  README.md                                          |  1 +
>>  3 files changed, 42 insertions(+), 18 deletions(-)
>>
>> -- 
>> 2.19.1.3.g30247aa5d201
>>

Re: [edk2-devel] [PATCH edk2-CCSS 3/3] must comment: add rule for documenting spurious variable assignments

[Philippe Mathieu-Daudé]

On 9/9/19 2:25 PM, Laszlo Ersek wrote:
> On 09/06/19 10:13, Philippe Mathieu-Daudé wrote:
>> Hi Laszlo,
>>
>> (Cc'ing Ard)
>>
>> On 9/5/19 8:38 PM, Laszlo Ersek wrote:
>>> Problem statement from Ard:
>>>
>>>> Sometimes, the GCC compiler warns about variables potentially being used
>>>> without having been initialized, while visual inspection reveals that
>>>> this is impossible. In such cases, we need to initialize such a variable
>>>> to an arbitrary value only to avoid breaking the build, given our policy
>>>> to treat warnings as errors.
>>
>> This is annoying.
>>
>> I suppose using CFLAGS+='-Wno-uninitialized -Wmaybe-uninitialized' is
>> not an acceptable option.
> 
> I don't have links handy, but around or before the time I filed
> TianoCore#607, we had gone through all the possibilities. The issue may
> have been possible to suppress with cmdline options for a particular
> toolchain version, but I'm fairly sure it was impossible to solve for
> all the toolchains simultaneously that edk2 supported at the time.

Oh, I see, this issue is old; I was not aware of EDK2 existence when it
was discussed.

>>>
>>> In such cases we generally use
>>>
>>>   LocalIntegerVariable = 0;
>>>
>>> and
>>>
>>>   LocalPointerVariable = NULL;
>>>
>>> which takes care of the incorrect warning. However, it also makes the
>>> human analysis of any subsequent logic harder, because it suggests that
>>> assigning that specific zero or NULL value to the local variable is
>>> *required* by the subsequent logic.
>>
>> What about having explicit definitions to silent warnings, so we don't
>> need to add comments?
>>
>> #define UNINITIALIZED_INTEGER 0
>> #define UNINITIALIZED_POINTER NULL
>>
>> Human review becomes trivial:
>>
>>    LocalPointerVariable = UNINITIALIZED_POINTER;
> 
> We did consider macros too, if I remember correctly. It was not liked.
> (We definitely considered magic values, see 0xDEADBEEF below, and those
> were clearly rejected.) People really seemed to want zero / NULL values,
> open-coded. I disagreed, but accepted. The explicit comment suggestion
> was a compromise from my side, therefore.
>
> In this patch set, I wouldn't like to introduce a rule that is not based
> in current practice. The code base is already full of the above kind of
> zero / NULL assignment; the only coding style detail, from the rule
> being suggested, is the comment.
> 
> While TianoCore#607 has been open, I've consistently directed developers
> to it, for the proposed syntax. Therefore, if you look at the code base
> today, you will find a large amount of the original un-annotated zero /
> NULL assignment (where you can't immediately tell whether they are
> algorithmically necessery or not), and a few instances of the wording
> proposed here.
> 
> $ git grep 'incorrect compiler/analyzer'
> 
> In that regard, this patch set aims to codify existing practice -- I
> just want to make the pattern more consistent.

OK I understand.

Your patch is an improvement regarding what we have today, enforcing a
cleaner codebase, so:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

I still wonder how many people rejected the macro proposal and agreed to
add comments instead, and why...
>From both my developer/reviewer point of view, the macros are obvious
and self-documented. Eventually we can restart the discussion regarding
using macros, and later use them. I'm not sure this is the best use of
our time.

Regards,

Phil.

>>
>>> In order to highlight such assignments, whose sole purpose is to suppress
>>> invalid "use before init" warnings from compilers or static analysis
>>> tools, we should mandate comments such as:
>>>
>>>   //
>>>   // set LocalVariable to suppress incorrect compiler/analyzer warnings
>>>   //
>>>   LocalVariable = 0;
>>>
>>> (Magic values such as 0xDEADBEEF, which would obviate the necessity of
>>> explicit comments, have been considered, and rejected for stylistic
>>> reasons.)
>>>
>>> Cc: Andrew Fish <afish@apple.com>
>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Cc: Rebecca Cran <rebecca@bsdio.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=607
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>  6_documenting_software/64_what_you_must_comment.md | 39 ++++++++++++++++++++
>>>  README.md                                          |  1 +
>>>  2 files changed, 40 insertions(+)
>>>
>>> diff --git a/6_documenting_software/64_what_you_must_comment.md b/6_documenting_software/64_what_you_must_comment.md
>>> index abb2114bf5bc..9e51c2e45816 100644
>>> --- a/6_documenting_software/64_what_you_must_comment.md
>>> +++ b/6_documenting_software/64_what_you_must_comment.md
>>> @@ -58,3 +58,42 @@ instance differs.
>>>  
>>>  When possible, you should also list the requirements that are satisfied by the
>>>  code.
>>> +
>>> +### 6.4.6 Comment spurious variable assignments.
>>> +
>>> +A compiler or static code analyzer may warn that an object with automatic or
>>> +allocated storage duration is read without having been initialized, while
>>> +visual inspection reveals that this is impossible.
>>> +
>>> +In order to suppress such a warning (which is emitted due to invalid data flow
>>> +analysis), developers explicitly assign the affected object the value to which
>>> +the same object would be initialized automatically, had the object static
>>> +storage duration, and no initializer. (The value assigned could be arbitrary;
>>> +the above-mentioned value is chosen for stylistic reasons.) For example:
>>> +
>>> +```c
>>> +UINTN LocalIntegerVariable;
>>> +VOID  *LocalPointerVariable;
>>> +
>>> +LocalIntegerVariable = 0;
>>> +LocalPointerVariable = NULL;
>>> +```
>>> +
>>> +This kind of assignment is difficult to distinguish from assignments where the
>>> +initial value of an object is meaningful, and is consumed by other code without
>>> +an intervening assignment. Therefore, each such assignment must be documented,
>>> +as follows:
>>> +
>>> +```c
>>> +UINTN LocalIntegerVariable;
>>> +VOID  *LocalPointerVariable;
>>> +
>>> +//
>>> +// set LocalIntegerVariable to suppress incorrect compiler/analyzer warnings
>>> +//
>>> +LocalIntegerVariable = 0;
>>> +//
>>> +// set LocalPointerVariable to suppress incorrect compiler/analyzer warnings
>>> +//
>>> +LocalPointerVariable = NULL;
>>> +```
>>> diff --git a/README.md b/README.md
>>> index e26133540368..0648819f0d3a 100644
>>> --- a/README.md
>>> +++ b/README.md
>>> @@ -113,3 +113,4 @@ Copyright (c) 2006-2017, Intel Corporation. All rights reserved.
>>>  | 2.2      | Convert to Gitbook                                                                                                                                | June 2017  |
>>>  |          | [#425](https://bugzilla.tianocore.org/show_bug.cgi?id=425) [CCS] clarify line breaking and indentation requirements for multi-line function calls |            |
>>>  |          | [#1656](https://bugzilla.tianocore.org/show_bug.cgi?id=1656) Update all Wiki pages for the BSD+Patent license change with SPDX identifiers        |            |
>>> +|          | [#607](https://bugzilla.tianocore.org/show_bug.cgi?id=607) Document code comment requirements for spurious variable assignments                   |            |
>>>
> 

Re: [PATCH edk2-CCSS 0/3] Coding Standards: add rule for documenting spurious variable assignments

[Leif Lindholm]

On Mon, Sep 09, 2019 at 02:35:15PM +0200, Laszlo Ersek wrote:
> On 09/06/19 14:26, Leif Lindholm wrote:
> > On Thu, Sep 05, 2019 at 08:38:17PM +0200, Laszlo Ersek wrote:
> >> Repo:   https://github.com/lersek/edk2-CCodingStandardsSpecification.git
> >> Branch: spurious_assign_bz_607
> >>
> >> HTML-rendered views of the modified pages:
> >> - https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607
> >> - https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/62_comments.html
> >> - https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/64_what_you_must_comment.html
> >>
> >> The first two patches are cleanups for things that popped up in the
> >> discussion in <https://bugzilla.tianocore.org/show_bug.cgi?id=607>.
> >>
> >> The third patch is the one fixing the BZ.
> > 
> > For 1 and 2,
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> > 
> > For 3, I see no issue with it, but I do feel tempted by Phil's input
> > of using explicit macros (obviating the need for specific comment).
> > I seem to recall back in the mists of time we considered something
> > similar.
> 
> Yes, I remember similarly.
> 
> > Vaguely. Am I misremembering, or did we disount that option?
> 
> Phil's current recommendation is what I would have preferred back then,
> but it was rejected, as far as I recall. If I remember correctly, most
> developers preferred naked NULLs / zeroes. I insisted on the comment as
> a fallback / compromise, so that we'd have at least some visual cue.

I'm not even sure I wasn't one of the people opposed to it then.
But if I was, I would appear to have changed my mind.

> I could be mis-remembering; we can restart that discussion if now the
> macros are preferred.

I would be all for that.
However, I see no reason why we shouldn't document the current process
in the meantime, so for 3/3 also:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Best Regards,

Leif

> Thanks,
> Laszlo
> 
> > 
> > Regards,
> > 
> > Leif
> > 
> >> Thanks,
> >> Laszlo
> >>
> >> Cc: Andrew Fish <afish@apple.com>
> >> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >> Cc: Rebecca Cran <rebecca@bsdio.com>
> >>
> >> Laszlo Ersek (3):
> >>   comments: remove "Horror Vacui" rule
> >>   comments: restrict and clarify applicability of "/*" comments
> >>   must comment: add rule for documenting spurious variable assignments
> >>
> >>  6_documenting_software/62_comments.md              | 20 +---------
> >>  6_documenting_software/64_what_you_must_comment.md | 39 ++++++++++++++++++++
> >>  README.md                                          |  1 +
> >>  3 files changed, 42 insertions(+), 18 deletions(-)
> >>
> >> -- 
> >> 2.19.1.3.g30247aa5d201
> >>
> 

Re: [edk2-devel] [PATCH edk2-CCSS 0/3] Coding Standards: add rule for documenting spurious variable assignments

[Ryszard Knop]

On Tue, 2019-09-10 at 16:33 +0100, Leif Lindholm wrote:
> On Mon, Sep 09, 2019 at 02:35:15PM +0200, Laszlo Ersek wrote:
> > On 09/06/19 14:26, Leif Lindholm wrote:
> > > On Thu, Sep 05, 2019 at 08:38:17PM +0200, Laszlo Ersek wrote:
> > > > Repo:   
> > > > https://github.com/lersek/edk2-CCodingStandardsSpecification.git
> > > > Branch: spurious_assign_bz_607
> > > > 
> > > > HTML-rendered views of the modified pages:
> > > > - 
> > > > https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607
> > > > - 
> > > > https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/62_comments.html
> > > > - 
> > > > https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/64_what_you_must_comment.html
> > > > 
> > > > The first two patches are cleanups for things that popped up in
> > > > the
> > > > discussion in <
> > > > https://bugzilla.tianocore.org/show_bug.cgi?id=607>;.
> > > > 
> > > > The third patch is the one fixing the BZ.
> > > 
> > > For 1 and 2,
> > > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > 
> > > For 3, I see no issue with it, but I do feel tempted by Phil's
> > > input
> > > of using explicit macros (obviating the need for specific
> > > comment).
> > > I seem to recall back in the mists of time we considered
> > > something
> > > similar.
> > 
> > Yes, I remember similarly.
> > 
> > > Vaguely. Am I misremembering, or did we disount that option?
> > 
> > Phil's current recommendation is what I would have preferred back
> > then,
> > but it was rejected, as far as I recall. If I remember correctly,
> > most
> > developers preferred naked NULLs / zeroes. I insisted on the
> > comment as
> > a fallback / compromise, so that we'd have at least some visual
> > cue.
> 
> I'm not even sure I wasn't one of the people opposed to it then.
> But if I was, I would appear to have changed my mind.
> 
> > I could be mis-remembering; we can restart that discussion if now
> > the
> > macros are preferred.
> 
> I would be all for that.

If my 2 cents are worth anything, that'd be preferred by some folks in
my team too. Although something shorter like "UNINITIALIZED_INT/PTR"
would be nicer, IMO. Both work of course.

Richard

> However, I see no reason why we shouldn't document the current
> process
> in the meantime, so for 3/3 also:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
> Best Regards,
> 
> Leif
> 
> > Thanks,
> > Laszlo
> > 
> > > Regards,
> > > 
> > > Leif
> > > 
> > > > Thanks,
> > > > Laszlo
> > > > 
> > > > Cc: Andrew Fish <afish@apple.com>
> > > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Cc: Rebecca Cran <rebecca@bsdio.com>
> > > > 
> > > > Laszlo Ersek (3):
> > > >   comments: remove "Horror Vacui" rule
> > > >   comments: restrict and clarify applicability of "/*" comments
> > > >   must comment: add rule for documenting spurious variable
> > > > assignments
> > > > 
> > > >  6_documenting_software/62_comments.md              | 20 +-----
> > > > ----
> > > >  6_documenting_software/64_what_you_must_comment.md | 39
> > > > ++++++++++++++++++++
> > > >  README.md                                          |  1 +
> > > >  3 files changed, 42 insertions(+), 18 deletions(-)
> > > > 
> > > > -- 
> > > > 2.19.1.3.g30247aa5d201
> > > > 
> 
> 
> 

Re: [edk2-devel] [PATCH edk2-CCSS 0/3] Coding Standards: add rule for documenting spurious variable assignments

[Laszlo Ersek]

On 09/10/19 17:44, Ryszard Knop wrote:
> On Tue, 2019-09-10 at 16:33 +0100, Leif Lindholm wrote:
>> On Mon, Sep 09, 2019 at 02:35:15PM +0200, Laszlo Ersek wrote:
>>> On 09/06/19 14:26, Leif Lindholm wrote:
>>>> On Thu, Sep 05, 2019 at 08:38:17PM +0200, Laszlo Ersek wrote:
>>>>> Repo:   
>>>>> https://github.com/lersek/edk2-CCodingStandardsSpecification.git
>>>>> Branch: spurious_assign_bz_607
>>>>>
>>>>> HTML-rendered views of the modified pages:
>>>>> - 
>>>>> https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607
>>>>> - 
>>>>> https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/62_comments.html
>>>>> - 
>>>>> https://lersek.gitbooks.io/laszlo-s-fork-of-the-edk-ii-c-coding-standards-sp/content/v/spurious_assign_bz_607/6_documenting_software/64_what_you_must_comment.html
>>>>>
>>>>> The first two patches are cleanups for things that popped up in
>>>>> the
>>>>> discussion in <
>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=607>;.
>>>>>
>>>>> The third patch is the one fixing the BZ.
>>>>
>>>> For 1 and 2,
>>>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>>>
>>>> For 3, I see no issue with it, but I do feel tempted by Phil's
>>>> input
>>>> of using explicit macros (obviating the need for specific
>>>> comment).
>>>> I seem to recall back in the mists of time we considered
>>>> something
>>>> similar.
>>>
>>> Yes, I remember similarly.
>>>
>>>> Vaguely. Am I misremembering, or did we disount that option?
>>>
>>> Phil's current recommendation is what I would have preferred back
>>> then,
>>> but it was rejected, as far as I recall. If I remember correctly,
>>> most
>>> developers preferred naked NULLs / zeroes. I insisted on the
>>> comment as
>>> a fallback / compromise, so that we'd have at least some visual
>>> cue.
>>
>> I'm not even sure I wasn't one of the people opposed to it then.
>> But if I was, I would appear to have changed my mind.
>>
>>> I could be mis-remembering; we can restart that discussion if now
>>> the
>>> macros are preferred.
>>
>> I would be all for that.
> 
> If my 2 cents are worth anything, that'd be preferred by some folks in
> my team too. Although something shorter like "UNINITIALIZED_INT/PTR"
> would be nicer, IMO. Both work of course.

Thanks everyone for the feedback thus far on this series. It looks like
I could go ahead and push the patches, minimally for bringing the CCSS
in closer sync with reality -- and then we could improve incrementally,
for example with macros.

But, before I push the set, I'd really like hear Mike's opinion too -- I
vaguely recall he was active in the original discussion. I wouldn't like
to back out the patches in case Mike rejected them retroactively.

I believe Mike will have a bit of an email backlog to process ;) so I'll
wait some more in this thread.

Thanks!
Laszlo

>> However, I see no reason why we shouldn't document the current
>> process
>> in the meantime, so for 3/3 also:
>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>
>> Best Regards,
>>
>> Leif
>>
>>> Thanks,
>>> Laszlo
>>>
>>>> Regards,
>>>>
>>>> Leif
>>>>
>>>>> Thanks,
>>>>> Laszlo
>>>>>
>>>>> Cc: Andrew Fish <afish@apple.com>
>>>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>>> Cc: Rebecca Cran <rebecca@bsdio.com>
>>>>>
>>>>> Laszlo Ersek (3):
>>>>>   comments: remove "Horror Vacui" rule
>>>>>   comments: restrict and clarify applicability of "/*" comments
>>>>>   must comment: add rule for documenting spurious variable
>>>>> assignments
>>>>>
>>>>>  6_documenting_software/62_comments.md              | 20 +-----
>>>>> ----
>>>>>  6_documenting_software/64_what_you_must_comment.md | 39
>>>>> ++++++++++++++++++++
>>>>>  README.md                                          |  1 +
>>>>>  3 files changed, 42 insertions(+), 18 deletions(-)
>>>>>
>>>>> -- 
>>>>> 2.19.1.3.g30247aa5d201
>>>>>
>>
>>
>>
> 
> 
> 
> 

Re: [edk2-devel] [PATCH edk2-CCSS 0/3] Coding Standards: add rule for documenting spurious variable assignments

[Michael D Kinney]

Series Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

I also agree that the macros would be cleaner, easy to review, and
and fewer lines of code without the comment block.  If I objected
previously, then I have also changed my mind.  I agree we can go
ahead and push the series in its current form and continue the
discussion on the macros.

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, September 11, 2019 10:51 AM
> To: devel@edk2.groups.io; ryszard.knop@linux.intel.com;
> leif.lindholm@linaro.org
> Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Rebecca Cran
> <rebecca@bsdio.com>; Philippe Mathieu-Daude
> <philmd@redhat.com>
> Subject: Re: [edk2-devel] [PATCH edk2-CCSS 0/3] Coding
> Standards: add rule for documenting spurious variable
> assignments
> 
> On 09/10/19 17:44, Ryszard Knop wrote:
> > On Tue, 2019-09-10 at 16:33 +0100, Leif Lindholm
> wrote:
> >> On Mon, Sep 09, 2019 at 02:35:15PM +0200, Laszlo
> Ersek wrote:
> >>> On 09/06/19 14:26, Leif Lindholm wrote:
> >>>> On Thu, Sep 05, 2019 at 08:38:17PM +0200, Laszlo
> Ersek wrote:
> >>>>> Repo:
> >>>>> https://github.com/lersek/edk2-
> CCodingStandardsSpecification.git
> >>>>> Branch: spurious_assign_bz_607
> >>>>>
> >>>>> HTML-rendered views of the modified pages:
> >>>>> -
> >>>>> https://lersek.gitbooks.io/laszlo-s-fork-of-the-
> edk-ii-c-coding-st
> >>>>> andards-sp/content/v/spurious_assign_bz_607
> >>>>> -
> >>>>> https://lersek.gitbooks.io/laszlo-s-fork-of-the-
> edk-ii-c-coding-st
> >>>>> andards-
> sp/content/v/spurious_assign_bz_607/6_documenting_softw
> are
> >>>>> /62_comments.html
> >>>>> -
> >>>>> https://lersek.gitbooks.io/laszlo-s-fork-of-the-
> edk-ii-c-coding-st
> >>>>> andards-
> sp/content/v/spurious_assign_bz_607/6_documenting_softw
> are
> >>>>> /64_what_you_must_comment.html
> >>>>>
> >>>>> The first two patches are cleanups for things
> that popped up in
> >>>>> the discussion in <
> >>>>>
> https://bugzilla.tianocore.org/show_bug.cgi?id=607>;.
> >>>>>
> >>>>> The third patch is the one fixing the BZ.
> >>>>
> >>>> For 1 and 2,
> >>>> Reviewed-by: Leif Lindholm
> <leif.lindholm@linaro.org>
> >>>>
> >>>> For 3, I see no issue with it, but I do feel
> tempted by Phil's
> >>>> input of using explicit macros (obviating the need
> for specific
> >>>> comment).
> >>>> I seem to recall back in the mists of time we
> considered something
> >>>> similar.
> >>>
> >>> Yes, I remember similarly.
> >>>
> >>>> Vaguely. Am I misremembering, or did we disount
> that option?
> >>>
> >>> Phil's current recommendation is what I would have
> preferred back
> >>> then, but it was rejected, as far as I recall. If I
> remember
> >>> correctly, most developers preferred naked NULLs /
> zeroes. I
> >>> insisted on the comment as a fallback / compromise,
> so that we'd
> >>> have at least some visual cue.
> >>
> >> I'm not even sure I wasn't one of the people opposed
> to it then.
> >> But if I was, I would appear to have changed my
> mind.
> >>
> >>> I could be mis-remembering; we can restart that
> discussion if now
> >>> the macros are preferred.
> >>
> >> I would be all for that.
> >
> > If my 2 cents are worth anything, that'd be preferred
> by some folks in
> > my team too. Although something shorter like
> "UNINITIALIZED_INT/PTR"
> > would be nicer, IMO. Both work of course.
> 
> Thanks everyone for the feedback thus far on this
> series. It looks like I could go ahead and push the
> patches, minimally for bringing the CCSS in closer sync
> with reality -- and then we could improve
> incrementally, for example with macros.
> 
> But, before I push the set, I'd really like hear Mike's
> opinion too -- I vaguely recall he was active in the
> original discussion. I wouldn't like to back out the
> patches in case Mike rejected them retroactively.
> 
> I believe Mike will have a bit of an email backlog to
> process ;) so I'll wait some more in this thread.
> 
> Thanks!
> Laszlo
> 
> >> However, I see no reason why we shouldn't document
> the current
> >> process in the meantime, so for 3/3 also:
> >> Reviewed-by: Leif Lindholm
> <leif.lindholm@linaro.org>
> >>
> >> Best Regards,
> >>
> >> Leif
> >>
> >>> Thanks,
> >>> Laszlo
> >>>
> >>>> Regards,
> >>>>
> >>>> Leif
> >>>>
> >>>>> Thanks,
> >>>>> Laszlo
> >>>>>
> >>>>> Cc: Andrew Fish <afish@apple.com>
> >>>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> >>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >>>>> Cc: Rebecca Cran <rebecca@bsdio.com>
> >>>>>
> >>>>> Laszlo Ersek (3):
> >>>>>   comments: remove "Horror Vacui" rule
> >>>>>   comments: restrict and clarify applicability of
> "/*" comments
> >>>>>   must comment: add rule for documenting spurious
> variable
> >>>>> assignments
> >>>>>
> >>>>>  6_documenting_software/62_comments.md
> | 20 +-----
> >>>>> ----
> >>>>>
> 6_documenting_software/64_what_you_must_comment.md | 39
> >>>>> ++++++++++++++++++++
> >>>>>  README.md
> |  1 +
> >>>>>  3 files changed, 42 insertions(+), 18
> deletions(-)
> >>>>>
> >>>>> --
> >>>>> 2.19.1.3.g30247aa5d201
> >>>>>
> >>
> >>
> >>
> >
> >
> > 
> >

Re: [edk2-devel] [PATCH edk2-CCSS 0/3] Coding Standards: add rule for documenting spurious variable assignments

[Laszlo Ersek]

On 09/17/19 21:10, Michael D Kinney wrote:
> Series Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> 
> I also agree that the macros would be cleaner, easy to review, and
> and fewer lines of code without the comment block.  If I objected
> previously, then I have also changed my mind.  I agree we can go
> ahead and push the series in its current form and continue the
> discussion on the macros.

Thank you all for the help, I've pushed the series:

d096859f15b9..f5ad35ec2c6d

Cheers,
Laszlo