Re: [PATCH v5 01/10] hw/virtio: Factorize virtio-mmio headers

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 10/3/19 6:26 AM, Sergio Lopez wrote:
>>
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>>> On 10/2/19 1:30 PM, Sergio Lopez wrote:
>>>> Put QOM and main struct definition in a separate header file, so it
>>>> can be accessed from other components.
>>>>
>>>> Signed-off-by: Sergio Lopez <slp@redhat.com>
>
>>>> +
>>>> +#ifndef QEMU_VIRTIO_MMIO_H
>>>> +#define QEMU_VIRTIO_MMIO_H
>>>
>>> I'd rather use HW_VIRTIO_MMIO_H
>>
>> Looks like there isn't a consensus in this regard:
>>
>> $ grep "ifndef" *
>
>>
>> Do we have an actual policy written somewhere?
>
> Past history shows several cleanups near commit fe2611b016, including
> commit c0a9956b which mentions scripts/clean-header-guards
> specifically for this purpose.  So yes, we have a policy, although it
> is not always enforced in a timely manner.

We haven't adopted a strict policy.

I created clean-header-guards.pl to help me tidy up the resulting mess
somewhat.  The script can clean up "untidy" header guards.  This may
involve replacing the guard symbol.  It derives the replacement symbol
from the file name the obvious way: convert a-z to A_Z, replace any
character that isn't okay in an identifier by '_'.  Guard symbols chosen
that way are fairly unlikely to collide.

Existing guard symbols often omit directories, and the script tolerates
that.  For instance, in sub/dir/base.h, anything ending in BASE_H is
considered tidy enough.  Might be a bad idea.

I wouldn't go as far as calling this a policy.  Perhaps it should be.


commit 2dbc4ebc1712a5cf9e6a36327dce0b465abd5bbe
Author: Markus Armbruster <armbru@redhat.com>
Date:   Tue Jun 28 13:07:36 2016 +0200

    scripts: New clean-header-guards.pl
    
    The conventional way to ensure a header can be included multiple times
    is to bracket it like this:
    
        #ifndef HEADER_NAME_H
        #define HEADER_NAME_H
        ...
        #endif
    
    where HEADER_NAME_H is a symbol unique to this header.
    
    The endif may be optionally decorated like this:
    
        #endif /* HEADER_NAME_H */
    
    Unconventional ways present in our code:
    
    * Identifiers reserved for any use:
        #define _FILEOP_H
    
    * Lowercase (bad idea for object-like macros):
        #define __linux_video_vga_h__
    
    * Roundabout ways to say the same thing (and hide from grep):
        #if !defined(__PPC_MAC_H__)
        #endif /* !defined(__PPC_MAC_H__) */
    
    * Redundant values:
        #define HW_ALPHA_H 1
    
    * Funny redundant values:
        # define PXA_H                 "pxa.h"
    
    * Decorations with bangs:
    
        #endif /* !QEMU_ARM_GIC_INTERNAL_H */
    
      The negation actually makes sense, but almost all our header guard
      #endif decorations don't negate.
    
    * Useless decorations:
    
       #endif  /* audio.h */
    
    Header guards are not the place to show off creativity.  This script
    normalizes them to the conventional way, and cleans up whitespace
    while there.  It warns when it renames guard symbols, and explains how
    to find occurences of these symbols that may have to be updated
    manually.
    
    Another issue is use of the same guard symbol in multiple headers.
    That's okay only for headers that cannot be used together, such as the
    *-user/*/target_syscall.h.  This script can't tell, so it warns when
    it sees a reuse.
    
    The script also warns when preprocessing a header with its guard
    symbol defined produces anything but whitespace.
    
    The next commits will put the script to use.
    
    Signed-off-by: Markus Armbruster <armbru@redhat.com>
    Reviewed-by: Richard Henderson <rth@twiddle.net>