| Message ID | 1283183960-28404-6-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. Paolo
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
On 08/30/2010 01:19 PM, Jes Sorensen wrote: > > 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. > Or, we exercise common sense instead of blinding removing arguments just because a certain uncommon warning mode of GCC complains. Regards, Anthony Liguori > Cheers, > Jes >
On 08/30/2010 11:27 AM, Anthony Liguori wrote: > On 08/30/2010 01:19 PM, Jes Sorensen wrote: >> >> 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. >> > > Or, we exercise common sense instead of blinding removing arguments > just because a certain uncommon warning mode of GCC complains. If you make a reasoned decision to keep the argument, then annotate it with #define UNUSED __attribute__((unused)) and the warning will go away. As to whether the argument should be retained in these specific cases, I am agnostic. r~
On 08/30/10 20:27, Anthony Liguori wrote: > On 08/30/2010 01:19 PM, Jes Sorensen wrote: >> >> 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. >> > > Or, we exercise common sense instead of blinding removing arguments just > because a certain uncommon warning mode of GCC complains. Before making the change, I did indeed look at the code for a while, considering whether it was reasonable to leave the unused variable in place. However I don't see anything in there that makes it likely that the block state parameter is going to be used in that function in the near future, if at all. If you feel so strongly about it, then lets apply the __unused attribute as Richard suggested so it's clear that that argument was added on purpose. Jes
diff --git a/block/raw.c b/block/raw.c index 61e6748..fc057d0 100644 --- a/block/raw.c +++ b/block/raw.c @@ -12,7 +12,7 @@ static int raw_open(BlockDriverState *bs, int flags) /* check for the user attempting to write something that looks like a block format header to the beginning of the image and fail out. */ -static int check_for_block_signature(BlockDriverState *bs, const uint8_t *buf) +static int check_for_block_signature(const uint8_t *buf) { static const uint8_t signatures[][4] = { { 'Q', 'F', 'I', 0xfb }, /* qcow/qcow2 */ @@ -42,7 +42,7 @@ static int check_write_unsafe(BlockDriverState *bs, int64_t sector_num, } if (sector_num == 0 && nb_sectors > 0) { - return check_for_block_signature(bs, buf); + return check_for_block_signature(buf); } return 0;