[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Headers without multiple inclusion guards
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] Headers without multiple inclusion guards |
Date: |
Mon, 3 Jun 2019 16:24:01 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 |
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'
Re: [Qemu-devel] Headers without multiple inclusion guards, Alistair Francis, 2019/06/05