Re: [Qemu-devel] [PATCH v8 19/23] SiFive RISC-V UART Device

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3695 bytes --]

On 04/10/2018 03:04 AM, Antony Pavlov wrote:

>>>> +++ b/include/hw/riscv/sifive_uart.h

>>>> +
>>>> +typedef struct SiFiveUARTState {
>>>> +    /*< private >*/
>>>> +    SysBusDevice parent_obj;
>>>
>>>
>>> You use SysBusDevive in this header file but there is no 'include "hw/sysbus.h"' in the header file itself.

That is, this header is not standalone; a .c file can't use the header
unless it first includes hw/sysbus.h prior to sifive_uart.h.

>>>
>>> Please see my comment https://github.com/riscv/riscv-qemu/pull/130#issuecomment-379640538
>>>
>>>
>>>> +    /*< public >*/
>>>> +    qemu_irq irq;
>>>> +    MemoryRegion mmio;
>>>> +    CharBackend chr;
>>>
>>> Just the same thing. CharBackend is defined in "chardev/char-fe.h" please include it.

If you were to use a CharBackend*, you could get by with just the
typedef.  Since all .c files include osdeps.h, which in turn includes
typedefs.h, you wouldn't have to include anything if you only refer to
the type via a pointer.  But here, you are including a full object, so
the compiler has to know the size of the type, which means this header
DOES depend on "chardev/char-fe.h" being included first (either in this
.h to keep it standalone, or in all .c files prior to the point where
they include sifive_uart.h).

>>
>> Honestly, I rather prefer to *not* add more includes to header files
>> than we already have. We already have got lots of "touch this header and
>> you have to recompile almost the whole QEMU" conditions, so to avoid
>> that this situation gets worse, we should rather avoid including headers
>> from headers if it is not necessary. Thus if the current sources build
>> fine, no need to change anything here. Just my 0.02 €.
> 
> Adding ONLY NECESSARY header files inclusions to header file __can't produce__
> additional recompile efforts.
> Moreover this can decrease number of include directives in c-files.

My personal preference: if your header only refers to a type via a
pointer where the header is still standalone with just the appropriate
typedefs, then DON'T include another .h from your header.  But if your
header has a hard dependency on something not already included by
osdeps.h, where failing to include that other header first creates a
compile error, then including the .h in your header is appropriate, as
it is less work for all .c clients that use your header.

The art of reducing compile-time dependencies is figuring out which
structs must be included inline (requiring .h in headers), and where you
can use pointers, or opaque types that live in .c, or whatever other
solutions, so that the headers become lighter-weight.  But it is NOT
designed to break standalone use of a header (other than the one
exception that headers DON'T include osdeps.h because that had to
already be included first by all .c files).

> 
> I __rebased__ my RISC-V board in my out-of tree qemu branch (https://github.com/miet-riscv-workgroup/riscv-qemu/tree/20180409.erizo). I faced with problem: I have to track dependencies of
> header files from include/hw/riscv/ which I use.
> 
> So your "not-add-more-includes-to-header-files" approach has an disadvantage:
> if a header file required header list changes, each c-file that includes that header file
> must be edited to update the #include statement list.

Indeed, that's why I argue that include statements in .h files that are
necessary for standalone compilation of that header is a good idea, and
not something to burden every .c file with.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]