Re: [Qemu-devel] [PATCH 1/4] block: vhdx - remove redundant comments

[Markus Armbruster]

Stefan Hajnoczi <address@hidden> writes:

> On Mon, Dec 08, 2014 at 01:07:42AM -0500, Jeff Cody wrote:
>> Minor cleanup.
>> 
>> Signed-off-by: Jeff Cody <address@hidden>
>> ---
>>  block/vhdx.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/block/vhdx.c b/block/vhdx.c
>> index 12bfe75..f1e1e2e 100644
>> --- a/block/vhdx.c
>> +++ b/block/vhdx.c
>> @@ -1109,8 +1109,8 @@ static coroutine_fn int vhdx_co_readv(BlockDriverState 
>> *bs, int64_t sector_num,
>>              /* check the payload block state */
>>              switch (s->bat[sinfo.bat_idx] & VHDX_BAT_STATE_BIT_MASK) {
>>              case PAYLOAD_BLOCK_NOT_PRESENT: /* fall through */

Please drop this one as well, it's 100% certified noise.

>> -            case PAYLOAD_BLOCK_UNDEFINED:   /* fall through */
>> -            case PAYLOAD_BLOCK_UNMAPPED:    /* fall through */
>> +            case PAYLOAD_BLOCK_UNDEFINED:
>> +            case PAYLOAD_BLOCK_UNMAPPED:
>>              case PAYLOAD_BLOCK_ZERO:
>>                  /* return zero */
>>                  qemu_iovec_memset(&hd_qiov, 0, 0, sinfo.bytes_avail);
>> @@ -1280,8 +1280,8 @@ static coroutine_fn int 
>> vhdx_co_writev(BlockDriverState *bs, int64_t sector_num,
>>  
>>                  /* fall through */

This is a keeper.

>>              case PAYLOAD_BLOCK_NOT_PRESENT: /* fall through */

This isn't.

>> -            case PAYLOAD_BLOCK_UNMAPPED:    /* fall through */
>> -            case PAYLOAD_BLOCK_UNDEFINED:   /* fall through */
>> +            case PAYLOAD_BLOCK_UNMAPPED:
>> +            case PAYLOAD_BLOCK_UNDEFINED:
>>                  bat_prior_offset = sinfo.file_offset;
>>                  ret = vhdx_allocate_block(bs, s, &sinfo.file_offset);
>>                  if (ret < 0) {
>
> Fall through comments are used by some static checkers.  Not sure if all
> checkers are smart enough to propagate the comment from adjacent case
> statements.  I would have left the comments alone but am okay with
> merging this.

I have never found a compiler or static checker so fanciful it actually
nags its users about the perfectly obvious and common pattern "several
case labels without code in between".

The problematic pattern is "control falls from one case's *code* through
to the next case".  That one needs a comment indeed.