[edk2-devel] [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 at apple.com>
Cc: Leif Lindholm <leif.lindholm at linaro.org>
Cc: Michael D Kinney <michael.d.kinney at intel.com>
Cc: Rebecca Cran <rebecca at bsdio.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=607
Signed-off-by: Laszlo Ersek <lersek at 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46935): https://edk2.groups.io/g/devel/message/46935
Mute This Topic: https://groups.io/mt/33157544/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-