[Qemu-devel] Re: [PATCH 05/14] Remove unused argument for check_for_block_signature()

[Jes Sorensen <Jes.Sorensen@redhat.com>]

On 08/30/10 18:40, Paolo Bonzini wrote:
> On 08/30/2010 06:16 PM, Anthony Liguori wrote:
>> This is why this type of warning sucks.  Passing BlockDriverState is a
>> matter of readability because these are roughly methods.  Just because
>> 'this' isn't used right now, doesn't mean that it should not be a method.
> 
> On the contrary, to me this is acceptable (or even "a good thing")
> because a patch introducing the first use of a so-far-unused argument
> deserves a more careful review.  In fact, if we were using C++,
> check_for_block_signature should have been static.
> 
> The cases where the "this" argument is unused in a method should stand
> out as possible bugs, as is the case with the parse errors in the JSON
> parser (which _is_ a bug, as the caller cannot intercept error messages
> right now).  check_for_block_signature is not one of these.

I totally agree on this. The problem with having such arguments passed
in is that you never know if they were used in the past and it was
forgotten when the code using them was removed, or if it's new code, in
which case they do deserve the extra scrutiny.

Cheers,
Jes