| Message ID | 20200418184111.13401-3-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fix -Wempty-body build warnings | expand |
On Sat, Apr 18, 2020 at 11:41 AM Randy Dunlap <rdunlap@infradead.org> wrote: > > Fix gcc empty-body warning when -Wextra is used: Please don't do this. First off, "do_empty()" adds nothing but confusion. Now it syntactically looks like it does something, and it's a new pattern to everybody. I've never seen it before. Secondly, even if we were to do this, then the patch would be wrong: > if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED) > - /* fall through */ ; > + do_empty(); /* fall through */ That comment made little sense before, but it makes _no_ sense now. What fall-through? I'm guessing it meant to say "nothing", and somebody was confused. With "do_empty()", it's even more confusing. Thirdly, there's a *reason* why "-Wextra" isn't used. The warnings enabled by -Wextra are usually complete garbage, and trying to fix them often makes the code worse. Exactly like here. Linus
On 4/18/20 11:53 AM, Linus Torvalds wrote: > On Sat, Apr 18, 2020 at 11:41 AM Randy Dunlap <rdunlap@infradead.org> wrote: >> >> Fix gcc empty-body warning when -Wextra is used: > > Please don't do this. > > First off, "do_empty()" adds nothing but confusion. Now it > syntactically looks like it does something, and it's a new pattern to > everybody. I've never seen it before. > > Secondly, even if we were to do this, then the patch would be wrong: > >> if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED) >> - /* fall through */ ; >> + do_empty(); /* fall through */ > > That comment made little sense before, but it makes _no_ sense now. > > What fall-through? I'm guessing it meant to say "nothing", and > somebody was confused. With "do_empty()", it's even more confusing. > > Thirdly, there's a *reason* why "-Wextra" isn't used. > > The warnings enabled by -Wextra are usually complete garbage, and > trying to fix them often makes the code worse. Exactly like here. OK, no problem. That's why PATCH 0/9 says RFC. Oops. Crap. It was *supposed* to say RFC. :(
> On Apr 18, 2020, at 1:53 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Thirdly, there's a *reason* why "-Wextra" isn't used. > > The warnings enabled by -Wextra are usually complete garbage, and > trying to fix them often makes the code worse. Exactly like here. > As the instigator of this warning cleanup activity, even _I_ don’t recommend building with all of -Wextra. Doing so on an allmodconfig build generates 500 megabytes of warning text (not exaggerating), primarily due to -Wunused-parameter and Wmissing-field-initializers. I strongly recommend disabling them with -Wno-unused-parameter -Wno-missing-field-initializers since the spew is completely unactionable. On the other hand, -Woverride-init found a legit bug that was breaking DVD drives, fixed in commit 03264ddde2453f6877a7d637d84068079632a3c5. I believe finding a good set of compiler warning settings can lead to lots of good bugs being spotted and (hopefully) fixed. Why let syzbot do all the work? zzy
--- linux-next-20200327.orig/fs/posix_acl.c +++ linux-next-20200327/fs/posix_acl.c @@ -124,7 +124,7 @@ struct posix_acl *get_acl(struct inode * * be an unlikely race.) */ if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED) - /* fall through */ ; + do_empty(); /* fall through */ /* * Normally, the ACL returned by ->get_acl will be cached.
Fix gcc empty-body warning when -Wextra is used: ../fs/posix_acl.c: In function ‘get_acl’: ../fs/posix_acl.c:127:22: warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body] /* fall through */ ; ^ Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: linux-fsdevel@vger.kernel.org Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Andrew Morton <akpm@linux-foundation.org> --- fs/posix_acl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)