Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/4] qapi/qnull: Add own header

[Max Reitz]

On 2017-06-21 18:24, Markus Armbruster wrote:
> Max Reitz <address@hidden> writes:
> 
>> Reviewed-by: Kevin Wolf <address@hidden>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  include/qapi/qmp/qnull.h   | 26 ++++++++++++++++++++++++++
>>  include/qapi/qmp/qobject.h |  8 --------
>>  include/qapi/qmp/types.h   |  1 +
>>  qobject/qnull.c            |  1 +
>>  target/i386/cpu.c          |  6 +-----
>>  tests/check-qnull.c        |  2 +-
>>  6 files changed, 30 insertions(+), 14 deletions(-)
>>  create mode 100644 include/qapi/qmp/qnull.h
>>
>> diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
>> new file mode 100644
>> index 0000000..69555ac
>> --- /dev/null
>> +++ b/include/qapi/qmp/qnull.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * QNull Module
>> + *
>> + * Copyright (C) 2009, 2017 Red Hat Inc.
>> + *
>> + * Authors:
>> + *  Luiz Capitulino <address@hidden>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
>> later.
>> + * See the COPYING.LIB file in the top-level directory.
> 
> Copy the boilerplate from qnull.c instead, factual correctness.

Sorry, will do.

>> + */
>> +
>> +#ifndef QNULL_H
>> +#define QNULL_H
>> +
>> +#include "qapi/qmp/qobject.h"
>> +
>> +extern QObject qnull_;
>> +
>> +static inline QObject *qnull(void)
>> +{
>> +    qobject_incref(&qnull_);
>> +    return &qnull_;
>> +}
>> +
>> +#endif /* QNULL_H */
> 
> Meh, another tiny header.  Are our compiles too fast?

For me, testing takes longer, so from my POV they're fast enough. ;-)

You can always merge all the QObject types into a single header; but as
the code is organized now, I think QNull should have its own header.

>> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
>> index b8ddbca..ef1d1a9 100644
>> --- a/include/qapi/qmp/qobject.h
>> +++ b/include/qapi/qmp/qobject.h
>> @@ -93,12 +93,4 @@ static inline QType qobject_type(const QObject *obj)
>>      return obj->type;
>>  }
>>  
>> -extern QObject qnull_;
>> -
>> -static inline QObject *qnull(void)
>> -{
>> -    qobject_incref(&qnull_);
>> -    return &qnull_;
>> -}
>> -
>>  #endif /* QOBJECT_H */
>> diff --git a/include/qapi/qmp/types.h b/include/qapi/qmp/types.h
>> index 27cfbd8..4c87182 100644
>> --- a/include/qapi/qmp/types.h
>> +++ b/include/qapi/qmp/types.h
>> @@ -20,5 +20,6 @@
>>  #include "qapi/qmp/qstring.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "qapi/qmp/qlist.h"
>> +#include "qapi/qmp/qnull.h"
>>  
>>  #endif /* QAPI_QMP_TYPES_H */
>> diff --git a/qobject/qnull.c b/qobject/qnull.c
>> index c124d05..b3cc85e 100644
>> --- a/qobject/qnull.c
>> +++ b/qobject/qnull.c
>> @@ -13,6 +13,7 @@
>>  #include "qemu/osdep.h"
>>  #include "qemu-common.h"
>>  #include "qapi/qmp/qobject.h"
> 
> Let's drop this include, like you do in check-qnull.c below.

I don't even know why I've dropped it there, must have been an accident.
But, well, I don't mind having more happy little accidents.

>> +#include "qapi/qmp/qnull.h"
>>  
>>  QObject qnull_ = {
>>      .type = QTYPE_QNULL,
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index b2b1d20..f118a54 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -29,11 +29,7 @@
>>  #include "qemu/option.h"
>>  #include "qemu/config-file.h"
>>  #include "qapi/qmp/qerror.h"
>> -#include "qapi/qmp/qstring.h"
>> -#include "qapi/qmp/qdict.h"
>> -#include "qapi/qmp/qbool.h"
>> -#include "qapi/qmp/qint.h"
>> -#include "qapi/qmp/qfloat.h"
>> +#include "qapi/qmp/types.h"
> 
> qapi/qmp/types.h is a lazy way to increase compile times by including
> more than you need.  One day I'll kill it.  Until then, I tolerate it in
> .c, but not in .h.

First, I think the compiler has enough work to do on i386/cpu.c as it
is, so it won't matter much here.

Secondly, I'll gladly take a qapi/qmp/primitive-types.h; but I don't see
the point of including six different headers and keeping track of which
primitives type you plan to use.

So if you want to merge them all into one, go for out, sounds very
reasonable to me.

>>  
>>  #include "qapi-types.h"
>>  #include "qapi-visit.h"
>> diff --git a/tests/check-qnull.c b/tests/check-qnull.c
>> index 8dd1c96..4a67b9a 100644
>> --- a/tests/check-qnull.c
>> +++ b/tests/check-qnull.c
>> @@ -8,7 +8,7 @@
>>   */
>>  #include "qemu/osdep.h"
>>  
>> -#include "qapi/qmp/qobject.h"
>> +#include "qapi/qmp/qnull.h"
>>  #include "qemu-common.h"
>>  #include "qapi/qobject-input-visitor.h"
>>  #include "qapi/qobject-output-visitor.h"
> 
> With the boilerplate corrected and the superfluous include in qnull.c
> deleted
> Reviewed-by: Markus Armbruster <address@hidden>

Will do, thanks!

Max

signature.asc
Description: OpenPGP digital signature