On 2020/7/21 下午9:55, David Sterba wrote: > On Tue, Jul 21, 2020 at 06:29:31PM +0800, Qu Wenruo wrote: >> On 2020/7/21 下午5:58, David Sterba wrote: >>> On Tue, Jul 21, 2020 at 07:51:00AM +0800, Qu Wenruo wrote: >>>> >>>> >>>> On 2020/7/21 上午12:09, David Sterba wrote: >>>>> On Mon, Jul 20, 2020 at 08:51:08PM +0800, Qu Wenruo wrote: >>>>>> --- a/convert/source-ext2.c >>>>>> +++ b/convert/source-ext2.c >>>>>> @@ -87,7 +87,8 @@ static int ext2_open_fs(struct btrfs_convert_context *cctx, const char *name) >>>>>> cctx->fs_data = ext2_fs; >>>>>> cctx->blocksize = ext2_fs->blocksize; >>>>>> cctx->block_count = ext2_fs->super->s_blocks_count; >>>>>> - cctx->total_bytes = ext2_fs->blocksize * ext2_fs->super->s_blocks_count; >>>>>> + cctx->total_bytes = (u64)ext2_fs->blocksize * >>>>>> + (u64)ext2_fs->super->s_blocks_count; >>>>> >>>>> Do you need to cast both? Once one of the types is wide enough for the >>>>> result, there should be no loss. >>>>> >>>> I just want to be extra safe. >>> >>> Typecasts in code raise questions why are they needed, 'to be extra' >>> safe is not a good reason. One typecast in multiplication/shifts is a >>> common pattern to widen the result but two look more like lack of >>> understanding of the integer promotion rules. >> >> My point here is, I don't want the reviewers or new contributors to >> bother about the promotion rules at all. > > Ouch, I hope you don't mean that contributors should ignore the trickier > parts of C language. Especially reviewers _have_ to bother about all > sorts of subtle behaviour. > >> They only need to know that using blocksize and blocks_count directly to >> do multiply would lead to overflow. >> >> Other details like whether the multiply follows the highest factor or >> the left operator or the right operator, shouldn't be the point and we >> don't really need to bother. > > ... and introduce bugs? > >> Thus casting both would definitely be right, without the need to refer >> to the complex rule book, thus save the reviewer several minutes. > > The opposite, if you send me code that's not following known schemes or > idiomatic schemes I'll be highly suspicious and looking for the reasons > why it's that way and making sure it's correct costs way more time. > OK, then would you please remove one casting at merge time, or do I need to resend? Thanks, Qu
