| Message ID | 20220518120137.2544-1-liupeibao@163.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/2] ext4: page-io: use 'unsigned int' to bare use of 'unsigned' | expand |
On Wed, May 18, 2022 at 08:01:36PM +0800, Liu Peibao wrote: > Fix warnings by checkpatch. > > Signed-off-by: Liu Peibao <liupeibao@163.com> Please don't send checkpatch-only patches. Thanks, - Ted
On 2022/6/16 22:49, Theodore Ts'o wrote: > On Wed, May 18, 2022 at 08:01:36PM +0800, Liu Peibao wrote: >> Fix warnings by checkpatch. >> >> Signed-off-by: Liu Peibao <liupeibao@163.com> > > Please don't send checkpatch-only patches. > > Thanks, > > - Ted Hi Ted, Thanks for your reply. What I want do to is rename some temporary variables in the patch2 and when I make the patch, there are the checkpatch warnings. From the point of view "one patch do one thing", I split the modification into two patches. Thanks! Best Regards, Peibao
On Sun, Jun 19, 2022 at 11:21:27AM +0800, Liu Peibao wrote: > > Thanks for your reply. What I want do to is rename some temporary variables > in the patch2 and when I make the patch, there are the checkpatch warnings. > From the point of view "one patch do one thing", I split the modification > into two patches. Thanks! I didn't really see the poiont of renaming the temporary variables, either. In this particular case basically only used to avoid line lengths from exceeding ~72 characters, and requiring a line wrap, and bio_start and bio_end is used only in one place in the code block below. Is it _really_ all that confusing whether they are named bio_{start,end} instead of bvec_{start,end}? If I was writing that code from scratch, I might have just used start and end without any prefixes. And as far as "only have a patch do one thing at a time", this doesn't apply to checkpatch fixes. The basic motivation behind "no checkpatch-only fixes" is that it tends to introduce code churn which makes interpreting information from "git blame" more difficult; and so therefore the costs exceed the extremely marginal benefits of fixing most checkpatch complaints. So making a _patch_ be checkpatch clean, whether it's modifying existing code or writing new code, is fine, since you're making a subtantive change to the code, so this is as good a time as any to fix up tiny nits such as checkpatch complaints. But the idea behind "no unnecessary code churn since it ruins git blame and could potentially induce future patch conflicts" also applies to renaming variables. The benefits are very minor, and they don't outweigh the costs. - Ted
On 6/20/22 2:18 AM, Theodore Ts'o wrote: > On Sun, Jun 19, 2022 at 11:21:27AM +0800, Liu Peibao wrote: >> >> Thanks for your reply. What I want do to is rename some temporary variables >> in the patch2 and when I make the patch, there are the checkpatch warnings. >> From the point of view "one patch do one thing", I split the modification >> into two patches. Thanks! > > I didn't really see the poiont of renaming the temporary variables, > either. > > In this particular case basically only used to avoid line lengths from > exceeding ~72 characters, and requiring a line wrap, and bio_start and > bio_end is used only in one place in the code block below. > > Is it _really_ all that confusing whether they are named > bio_{start,end} instead of bvec_{start,end}? > > If I was writing that code from scratch, I might have just used start > and end without any prefixes. And as far as "only have a patch do one > thing at a time", this doesn't apply to checkpatch fixes. > > The basic motivation behind "no checkpatch-only fixes" is that it > tends to introduce code churn which makes interpreting information > from "git blame" more difficult; and so therefore the costs exceed the > extremely marginal benefits of fixing most checkpatch complaints. So > making a _patch_ be checkpatch clean, whether it's modifying existing > code or writing new code, is fine, since you're making a subtantive > change to the code, so this is as good a time as any to fix up tiny > nits such as checkpatch complaints. > > But the idea behind "no unnecessary code churn since it ruins git > blame and could potentially induce future patch conflicts" also > applies to renaming variables. The benefits are very minor, and they > don't outweigh the costs. > > - Ted > Got it! Thanks for your detailed and comprehensive explanation! Best Regards, Peibao
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c index 14695e2b5042..fd55e11c8391 100644 --- a/fs/ext4/page-io.c +++ b/fs/ext4/page-io.c @@ -106,9 +106,9 @@ static void ext4_finish_bio(struct bio *bio) struct page *page = bvec->bv_page; struct page *bounce_page = NULL; struct buffer_head *bh, *head; - unsigned bio_start = bvec->bv_offset; - unsigned bio_end = bio_start + bvec->bv_len; - unsigned under_io = 0; + unsigned int bio_start = bvec->bv_offset; + unsigned int bio_end = bio_start + bvec->bv_len; + unsigned int under_io = 0; unsigned long flags; if (fscrypt_is_bounce_page(page)) { @@ -329,7 +329,7 @@ static void ext4_end_bio(struct bio *bio) if (WARN_ONCE(!io_end, "io_end is NULL: %pg: sector %Lu len %u err %d\n", bio->bi_bdev, (long long) bio->bi_iter.bi_sector, - (unsigned) bio_sectors(bio), + (unsigned int) bio_sectors(bio), bio->bi_status)) { ext4_finish_bio(bio); bio_put(bio); @@ -435,7 +435,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io, { struct page *bounce_page = NULL; struct inode *inode = page->mapping->host; - unsigned block_start; + unsigned int block_start; struct buffer_head *bh, *head; int ret = 0; int nr_submitted = 0;
Fix warnings by checkpatch. Signed-off-by: Liu Peibao <liupeibao@163.com> --- fs/ext4/page-io.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)