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 <af...@apple.com> > Cc: Leif Lindholm <leif.lindh...@linaro.org> > Cc: Michael D Kinney <michael.d.kin...@intel.com> > Cc: Rebecca Cran <rebe...@bsdio.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=607 > Signed-off-by: Laszlo Ersek <ler...@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 > | | > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46968): https://edk2.groups.io/g/devel/message/46968 Mute This Topic: https://groups.io/mt/33157544/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
