Re: [Qemu-devel] Headers without multiple inclusion guards

[Philippe Mathieu-Daudé]

On 6/3/19 2:59 PM, Markus Armbruster wrote:
> Laszlo Ersek <address@hidden> writes:
> 
>> Hi Markus,
>>
>> (sorry about the late reply, I've been away.)
>>
>> On 05/28/19 20:12, Markus Armbruster wrote:
>>
>>> EDK2 Firmware
>>> M: Laszlo Ersek <address@hidden>
>>> M: Philippe Mathieu-Daudé <address@hidden>
>>> tests/uefi-test-tools/UefiTestToolsPkg/Include/Guid/BiosTablesTest.h
>>
>> This header file does have a multiple inclusion guard:
>>
>>> /** @file
>>>   Expose the address(es) of the ACPI RSD PTR table(s) and the SMBIOS entry
>>>   point(s) in a MB-aligned structure to the hypervisor.
>>>
>>>   [...]
>>> **/
>>>
>>> #ifndef __BIOS_TABLES_TEST_H__
>>> #define __BIOS_TABLES_TEST_H__
>>>
>>> [...]
>>>
>>> #endif // __BIOS_TABLES_TEST_H__
>>
>> It's possible that "scripts/clean-header-guards.pl" does not recognize
>> the guard.
> 
> Correct.  I fixed the script in my tree.
> 
>> According to the ISO C standard, "All identifiers that begin with an
>> underscore and either an uppercase letter or another underscore are
>> always reserved for any use". Therefore, technically speaking, the above
>> inclusion guard implies undefined behavior. In practice, this particular
>> style for header guards is extremely common in the edk2 codebase:
>>
>> $ git grep '^#ifndef __' -- '*.h'  | wc -l
>> 1012
>>
>> And, "tests/uefi-test-tools/UefiTestToolsPkg" follows the edk2 coding
>> style.
>>
>> That said, if you'd like to remove the leading "__" from the macro name,
>> I'd be fully OK with that.
> 
> We routinely exempt files from style cleanups when we have a reason.  If
> you want this one to be exempted, that's fine with me.
> 
> If we decide not to exempt it, then I want a header guard that makes my
> (fixed) script happy.  It isn't right now:
> 
>     $ scripts/clean-header-guards.pl -nv 
> tests/uefi-test-tools/UefiTestToolsPkg/Include/Guid/BiosTablesTest.h 
>     tests/uefi-test-tools/UefiTestToolsPkg/Include/Guid/BiosTablesTest.h 
> guard __BIOS_TABLES_TEST_H__ needs cleanup:
>         is a reserved identifier, doesn't end with _H, doesn't match the file 
> name
>     [...]
> 
> Removing the leading "__" takes care of the first complaint:
> 
>     tests/uefi-test-tools/UefiTestToolsPkg/Include/Guid/BiosTablesTest.h 
> guard BIOS_TABLES_TEST_H__ needs cleanup:
>         doesn't end with _H, doesn't match the file name
> 
> Removing the trailing "__" as well takes care of the second one:
> 
>     tests/uefi-test-tools/UefiTestToolsPkg/Include/Guid/BiosTablesTest.h 
> guard BIOS_TABLES_TEST_H needs cleanup:
>         doesn't match the file name
> 
> To get rid of the last one, we can:
> 
> * Rename to BIOSTABLESTEST_H.  Easy.
> 
> * Teach scripts/clean-header-guards.pl to capitalize StudlyCaps
>   filenames to STUDLY_CAPS rather than STUDLYCAPS.  But that would break
>   include/libdecnumber/*.h.
> 
> * Teach scripts/clean-header-guards to accept either STUDLYCAPS or
>   STUDLY_CAPS.  Considering we have exactly one filename that needs
>   this, I'd prefer not to.
> 
> My first preference is BIOSTABLESTEST_H, second is to exempt the file.
> Yours?
> 

What about excluding UefiTestToolsPkg?

$ git grep '^#ifndef __' -- \
  '*.h' ':!tests/uefi-test-tools/UefiTestToolsPkg'