Drop -Wdeclaration-after-statement
diff mbox series

Message ID 20190310133535.GA20473@avx2
State New, archived
Headers show
Series
  • Drop -Wdeclaration-after-statement
Related show

Commit Message

Alexey Dobriyan March 10, 2019, 1:35 p.m. UTC
Newly added static_assert() is formally a declaration, which will give
a warning if used in the middle of the function.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 Makefile |    3 ---
 1 file changed, 3 deletions(-)

Comments

Andrew Morton March 12, 2019, 12:38 a.m. UTC | #1
On Sun, 10 Mar 2019 16:35:35 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> Newly added static_assert() is formally a declaration, which will give
> a warning if used in the middle of the function.
> 
> ...
>
> --- a/Makefile
> +++ b/Makefile
> @@ -792,9 +792,6 @@ endif
>  # arch Makefile may override CC so keep this after arch Makefile is included
>  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
>  
> -# warn about C99 declaration after statement
> -KBUILD_CFLAGS += -Wdeclaration-after-statement
> -
>  # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
>  KBUILD_CFLAGS += $(call cc-option,-Wvla)

I do wish your changelogs were more elaborate :(

So the proposal is to disable -Wdeclaration-after-statement in all
cases for all time because static_assert() doesn't work correctly?

Surely there's something we can do to squish the static_assert() issue
while retaining -Wdeclaration-after-statement?  Perhaps by making
static_assert() a nop if -Wdeclaration-after-statement is in use. 
Perhaps simply by putting { } around the static_assert()?
Alexey Dobriyan March 12, 2019, 5:24 p.m. UTC | #2
On Mon, Mar 11, 2019 at 05:38:45PM -0700, Andrew Morton wrote:
> On Sun, 10 Mar 2019 16:35:35 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > Newly added static_assert() is formally a declaration, which will give
> > a warning if used in the middle of the function.
> > 
> > ...
> >
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -792,9 +792,6 @@ endif
> >  # arch Makefile may override CC so keep this after arch Makefile is included
> >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> >  
> > -# warn about C99 declaration after statement
> > -KBUILD_CFLAGS += -Wdeclaration-after-statement
> > -
> >  # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> >  KBUILD_CFLAGS += $(call cc-option,-Wvla)
> 
> I do wish your changelogs were more elaborate :(

> So the proposal is to disable -Wdeclaration-after-statement in all
> cases for all time because static_assert() doesn't work correctly?

Yes. I converted 2 cases in /proc to static_assert() and you can't write

	{
		[code]
		static_assert()
	}

without a warning because static_assert() is declaration.
So people would move BUILD_BUG_ON() to where it doesn't belong.

> Surely there's something we can do to squish the static_assert() issue
> while retaining -Wdeclaration-after-statement?

It is not good in my opinion to stick to -Wdeclaration-after-statement.

> Perhaps by making
> static_assert() a nop if -Wdeclaration-after-statement is in use. 
> Perhaps simply by putting { } around the static_assert()?

Making a statement out of it would disable current cases where it is
placed in headers.
Andrew Morton March 12, 2019, 7:50 p.m. UTC | #3
On Tue, 12 Mar 2019 20:24:47 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Mon, Mar 11, 2019 at 05:38:45PM -0700, Andrew Morton wrote:
> > On Sun, 10 Mar 2019 16:35:35 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > 
> > > Newly added static_assert() is formally a declaration, which will give
> > > a warning if used in the middle of the function.
> > > 
> > > ...
> > >
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -792,9 +792,6 @@ endif
> > >  # arch Makefile may override CC so keep this after arch Makefile is included
> > >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> > >  
> > > -# warn about C99 declaration after statement
> > > -KBUILD_CFLAGS += -Wdeclaration-after-statement
> > > -
> > >  # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> > >  KBUILD_CFLAGS += $(call cc-option,-Wvla)
> > 
> > I do wish your changelogs were more elaborate :(
> 
> > So the proposal is to disable -Wdeclaration-after-statement in all
> > cases for all time because static_assert() doesn't work correctly?
> 
> Yes. I converted 2 cases in /proc to static_assert() and you can't write
> 
> 	{
> 		[code]
> 		static_assert()
> 	}
> 
> without a warning because static_assert() is declaration.
> So people would move BUILD_BUG_ON() to where it doesn't belong.

Sure.

> > Surely there's something we can do to squish the static_assert() issue
> > while retaining -Wdeclaration-after-statement?
> 
> It is not good in my opinion to stick to -Wdeclaration-after-statement.

Why?

> > Perhaps by making
> > static_assert() a nop if -Wdeclaration-after-statement is in use. 
> > Perhaps simply by putting { } around the static_assert()?
> 
> Making a statement out of it would disable current cases where it is
> placed in headers.

I think you mean cases where static_assert() is used outside functions?

We could have two versions of it, one for use inside functions, one for
use outside functions?
Alexey Dobriyan March 12, 2019, 8:18 p.m. UTC | #4
On Tue, Mar 12, 2019 at 12:50:17PM -0700, Andrew Morton wrote:
> On Tue, 12 Mar 2019 20:24:47 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > On Mon, Mar 11, 2019 at 05:38:45PM -0700, Andrew Morton wrote:
> > > On Sun, 10 Mar 2019 16:35:35 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > > 
> > > > Newly added static_assert() is formally a declaration, which will give
> > > > a warning if used in the middle of the function.
> > > > 
> > > > ...
> > > >
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -792,9 +792,6 @@ endif
> > > >  # arch Makefile may override CC so keep this after arch Makefile is included
> > > >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> > > >  
> > > > -# warn about C99 declaration after statement
> > > > -KBUILD_CFLAGS += -Wdeclaration-after-statement
> > > > -
> > > >  # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> > > >  KBUILD_CFLAGS += $(call cc-option,-Wvla)
> > > 
> > > I do wish your changelogs were more elaborate :(
> > 
> > > So the proposal is to disable -Wdeclaration-after-statement in all
> > > cases for all time because static_assert() doesn't work correctly?
> > 
> > Yes. I converted 2 cases in /proc to static_assert() and you can't write
> > 
> > 	{
> > 		[code]
> > 		static_assert()
> > 	}
> > 
> > without a warning because static_assert() is declaration.
> > So people would move BUILD_BUG_ON() to where it doesn't belong.
> 
> Sure.
> 
> > > Surely there's something we can do to squish the static_assert() issue
> > > while retaining -Wdeclaration-after-statement?
> > 
> > It is not good in my opinion to stick to -Wdeclaration-after-statement.
> 
> Why?

It is useful to have declarations mixed with code.
It reduces effective scope of a variable:

	int a;
	[a misused]
		...
	[a used correctly]

vs

	[a misused -- compile error]
		...
	int a;
	[a used correctly]

It is possible to partially workaround that but at the cost of a
indentation level. I'll post the following patch soon:

-       NEW_AUX_ENT(AT_UID, from_kuid_munged(cred->user_ns, cred->uid));
-       NEW_AUX_ENT(AT_EUID, from_kuid_munged(cred->user_ns, cred->euid));
-       NEW_AUX_ENT(AT_GID, from_kgid_munged(cred->user_ns, cred->gid));
-       NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid));
+       {
+               const struct cred *cred = current_cred();
+               struct user_namespace *user_ns = cred->user_ns;
+
+               NEW_AUX_ENT(AT_UID,  from_kuid_munged(user_ns, cred->uid));
+               NEW_AUX_ENT(AT_EUID, from_kuid_munged(user_ns, cred->euid));
+               NEW_AUX_ENT(AT_GID,  from_kgid_munged(user_ns, cred->gid));
+               NEW_AUX_ENT(AT_EGID, from_kgid_munged(user_ns, cred->egid));
+       }

Often it is simply not possible to shift big function one level
deeper.

Another related thing, C99 has this very cool feature of per-for-loop
declarations:

	for (int i = 0; ...)

Once kernel will switch to C99 or C11 it _will_ be used to the point of
requiring it on the coding style level. The superstition of declaring
everything in the beginning of a function will fall, so might as well
start earlier.
Alexey Dobriyan March 12, 2019, 8:24 p.m. UTC | #5
On Tue, Mar 12, 2019 at 12:50:17PM -0700, Andrew Morton wrote:
> On Tue, 12 Mar 2019 20:24:47 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > On Mon, Mar 11, 2019 at 05:38:45PM -0700, Andrew Morton wrote:
> > > On Sun, 10 Mar 2019 16:35:35 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > > 
> > > > Newly added static_assert() is formally a declaration, which will give
> > > > a warning if used in the middle of the function.
> > > > 
> > > > ...
> > > >
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -792,9 +792,6 @@ endif
> > > >  # arch Makefile may override CC so keep this after arch Makefile is included
> > > >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> > > >  
> > > > -# warn about C99 declaration after statement
> > > > -KBUILD_CFLAGS += -Wdeclaration-after-statement
> > > > -
> > > >  # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> > > >  KBUILD_CFLAGS += $(call cc-option,-Wvla)
> > > 
> > > I do wish your changelogs were more elaborate :(
> > 
> > > So the proposal is to disable -Wdeclaration-after-statement in all
> > > cases for all time because static_assert() doesn't work correctly?
> > 
> > Yes. I converted 2 cases in /proc to static_assert() and you can't write
> > 
> > 	{
> > 		[code]
> > 		static_assert()
> > 	}
> > 
> > without a warning because static_assert() is declaration.
> > So people would move BUILD_BUG_ON() to where it doesn't belong.
> 
> Sure.
> 
> > > Surely there's something we can do to squish the static_assert() issue
> > > while retaining -Wdeclaration-after-statement?
> > 
> > It is not good in my opinion to stick to -Wdeclaration-after-statement.
> 
> Why?
> 
> > > Perhaps by making
> > > static_assert() a nop if -Wdeclaration-after-statement is in use. 
> > > Perhaps simply by putting { } around the static_assert()?
> > 
> > Making a statement out of it would disable current cases where it is
> > placed in headers.
> 
> I think you mean cases where static_assert() is used outside functions?
> 
> We could have two versions of it, one for use inside functions, one for
> use outside functions?

I don't know. It was made a declaration in the standard specifically
to have only one name which is a good thing.

Just count the number of BUILD_BUG_* variants, it is ridiculous for such
a simple thing. Or even better, all *alloc* variants. :^)
Andrew Morton March 12, 2019, 8:53 p.m. UTC | #6
> > > 
> > > It is not good in my opinion to stick to -Wdeclaration-after-statement.
> > 
> > Why?
> 
> It is useful to have declarations mixed with code.

Am inclined to agree.  Maybe.

> Once kernel will switch to C99 or C11 it _will_ be used to the point of
> requiring it on the coding style level. The superstition of declaring
> everything in the beginning of a function will fall, so might as well
> start earlier.

Sure, it's a matter of kernel coding conventions.

But this isn't the way to get those changed!  The process for making such
changes involves large numbers of people arguing at each other (perhaps
at kernel summit) until Linus comes in an tells everyone how it's going
to be.

Patch
diff mbox series

--- a/Makefile
+++ b/Makefile
@@ -792,9 +792,6 @@  endif
 # arch Makefile may override CC so keep this after arch Makefile is included
 NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
 
-# warn about C99 declaration after statement
-KBUILD_CFLAGS += -Wdeclaration-after-statement
-
 # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
 KBUILD_CFLAGS += $(call cc-option,-Wvla)