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

[Laszlo Ersek]

On 06/03/19 16:24, Philippe Mathieu-Daudé wrote:
> 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'
> 

Let's go with BIOSTABLESTEST_H for now. If UefiTestToolsPkg continues to
generate a bunch of warnings in other QEMU checkers too, we can still
decide to exclude it, later. This change looks small enough to me, for now.

BTW thanks for that nice git-grep syntax :) I guess I should read up on
"pathspec" in gitglossary(7) sometime...

Thanks!
Laszlo