document code comment requirements for spurious variable assignments (Bugzilla Bug 607)

[tianocore-issues]

This issue was created automatically with bugzilla2github

Bugzilla Bug 607

Date: 2017-06-21T18:02:50+00:00
From: @lersek
To: @lersek
CC: @ardbiesheuvel, @lersek, nobody, @leiflindholm, @bexcran, yonghong.zhu

Last updated: 2019-09-18T06:18:43+00:00

[tianocore-issues]

Comment 2308

Date: 2017-06-21 18:02:50 +0000
From: @lersek

• Industry Specification: ---
• Releases to Fix: EDK II Master
• Target OS: ---
• Bugzilla Assignee(s): @lersek

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 warnigns
//
LocalVariable = 0;

[tianocore-issues]

Comment 2309

Date: 2017-06-21 18:11:44 +0000
From: @lersek

A list of commits from OvmfPkg where such code was added (in a dedicated
patch, or else where an independent patch was fixed up to suppress the
warning):

32a22f09d795 ("OvmfPkg: QemuBootOrder: collect active UEFI boot options in
advance", 2013-09-13)
bc4c536628c6 ("OvmfPkg/PlatformDxe: Silence warning seen with GCC48 IA32",
2014-03-22)
8c01a99b8472 ("OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix GCC44 build
failure.", 2014-06-27)
ea5396f31a8d ("Fix VS2013 build failure.", 2014-08-13)
02d6f4ce0c84 ("OvmfPkg/PlatformPei: suppress wrong VS2008 warning (use of
uninited local)", 2016-03-19)
8e8fd3037788 ("OvmfPkg/PlatformBootManagerLib: Fix gcc-4.8 Ia32 build
failure", 2016-05-03)
8f98c76f992b ("OvmfPkg/AcpiPlatformDxe: fix spurious uninitialized var
warning", 2017-06-08)

In these patches code comments of the suggested form were not added.

More such assignments exist (for example, see
"ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c":

//
// The following output arguments are initialized only in
// order to suppress '-Werror=maybe-uninitialized' warnings
// incorrectly emitted by some gcc versions.
//

) but those that I could find are actually commented.

[tianocore-issues]

Comment 2326

Date: 2017-06-22 20:11:38 +0000
From: Yonghong Zhu <<yonghong.zhu>>

Bug scrub: Assign to module owner

[tianocore-issues]

Comment 2423

Date: 2017-07-11 18:02:30 +0000
From: @lersek

(In reply to Laszlo Ersek from comment #0)

> //
> // set LocalVariable to suppress incorrect compiler/analyzer warnigns
> //
> LocalVariable = 0;

Please, if you copy the above example into live code, be advised that in comment 0, I made a typo -- I mistyped "warning" as "warnign". (Obviously the best place to make a typo is one that people will copy on purpose.)

So the example is, with the typo fixed:

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

[tianocore-issues]

Comment 2724

Date: 2017-09-12 20:24:13 +0000
From: @bexcran

§6.2.3 of the EDKII C Coding Standards (draft 6/13/2017) say not to have comment characters alone on a line.

From https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/6_documenting_software/62_comments.html :

6.2.3 Avoid comments where the opening comment characters are alone on a line.

/*

• VIOLATION: Horror Vacui
  */
  or

//
// VIOLATION: Horror Vacui
//

[tianocore-issues]

Comment 2727

Date: 2017-09-13 16:39:11 +0000
From: @lersek

Rebecca, I totally don't understand that comment. What does the expression "opening comment characters" denote?

If it means

// <------------- THIS
// blah
//

then that rule should simply be removed from the coding style spec, because it is wrong. The edk2 tree is full of this kind of comment; it is actually required from contributors.

... Looking more at the same part of the CCS, I find:

6.2.1 Only use C style, "/*", comments for multi-line comments and on the same line as pre-processor directives.

I'm dumbfounded by this too. C style "/*" comments are exclusively used, in practice, in top-of-file comment blocks, and function-level comment blocks. Type definitions, global variables, and function-scope comments are always written with //.

I'm stunned by these sections of the CCS because everyday development practice is 100% opposed to it. I mean if you posted a patch that went like

Variable1 = 2;
// let's do this
Variable2 = 3;

or

Variable1 = 2;
/* let's do this */
Variable2 = 3;

as opposed to

Variable1 = 2;
//
// let's do this
//
Variable2 = 3;

the maintainer reviewing the code (regardless of Package) would absolutely not accept the comment style.

... See for example

https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/6_documenting_software/65_types_of_comments.html#65-types-of-comments

"6.5.2.1 For internal code comments, use C++ style (//) comment lines."

It conflicts with 6.2.1.

Another example, from "6.5.2.5 Indent the comments with the code.":

// Only process data if mTest is TRUE.
// This comment block applies to the entire if/else statement.
if (mTest) {
// This is an example comment to explain why this behavior
// is appropriate if mTest is true.
// This comment block only applies when mTest is true.

This conflicts with 6.2.3.

I think 6.2.1 and 6.2.3 should be removed; they are dead rules.

[tianocore-issues]

Comment 9303

Date: 2019-08-27 18:53:21 +0000
From: @lersek

I'll try to find some time for sending CCS patches for this. Taking the BZ.

[tianocore-issues]

Comment 9411

Date: 2019-08-30 08:36:47 +0000
From: @lersek

Ready to send some patches, but ran into a snag with gitbook.com:

[edk2-devel] gitbook.com integration doesn't seem to work
http://mid.mail-archive.com/275f3a9f-85d9-7585-fbe6-4cff8e62fdda@redhat.com
https://edk2.groups.io/g/devel/message/46618

[tianocore-issues]

Comment 9478

Date: 2019-09-05 14:40:23 +0000
From: @lersek

Posted:

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

http://mid.mail-archive.com/20190905183820.10312-1-lersek@redhat.com
https://edk2.groups.io/g/devel/message/46932

[tianocore-issues]

Comment 9706

Date: 2019-09-18 06:18:43 +0000
From: @lersek

Fixed in edk2-CCodingStandardsSpecification.git, commit range d096859..f5ad35e.