Re: [Qemu-devel] [PATCH v5 07/13] libqtest: Inline qtest_query_target_endianness()

[Eric Blake]

On 08/18/2017 04:46 PM, Philippe Mathieu-Daudé wrote:
> On 08/18/2017 06:15 PM, Eric Blake wrote:
>> There was only one caller; it's easier to inline things.
> 
> What's the benefit of this change? Having a separate function ease code
> reading. You might add the 'inline' qualifier but the compiler is smart
> enough to inline it.
> 
> Instead of this change I'd inline it and remove the return value, having
> a void function initializing s->big_endian. Matter of taste I think :)
> 
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>   tests/libqtest.c | 22 ++++++----------------
>>   1 file changed, 6 insertions(+), 16 deletions(-)

I think the diffstat shows that inlining is a win.  My other reason that
it is a win:

>> @@ -351,8 +338,11 @@ QTestState
>> *qtest_init_without_qmp_handshake(const char *extra_args)
>>       }
>>
>>       /* ask endianness of the target */
>> -
>> -    s->big_endian = qtest_query_target_endianness(s);
>> +    qtest_sendf(s, "endianness\n");
>> +    args = qtest_rsp(s, 1);

The old code has to pass an arbitrary QTestState *s down to a helper
function.  But I'm trying to get rid of as many functions as possible
that depend on arbitrary QTestState *s, particularly when callers end up
passing 'global_qtest' for 's'.  Inlining it makes it more obvious that
the changes in 10/13 are safely operating on global_qtest, which in turn
makes it possible for 12/13 to drop QTestState *s parameters from
qtest_sendf() and qtest_rsp().  If the function were not inlined, those
later cleanups would not be as trivial.  (Or put another way - I rebased
this patch to be earlier in the series precisely because of what I ran
into while writing those patches)

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

signature.asc
Description: OpenPGP digital signature