Cherry-picking C99 features (was: [PATCH] xx)

[Ralph Boehme]

Hey metze!

On Thu, Apr 28, 2016 at 12:53:35AM +0200, Stefan Metzmacher wrote:
> slow wrote:
> > What can we do?
> > 
> > If we decide we *don't* want to support specific C99 features like
> > mixed declaration and code, there's just no way to say to the
> > compiler "I want C99 feaure A but I don't want C99 feature B, please
> > warn me if I inadvertently use it".
> 
> We're using -Werror=declaration-after-statement which seems to do the job
> to prevent such patches to pass autobuild.

unfortunately not:

[slow at kazak ~]$ cat test.c
int main(int argc, char **argv)
{
        for (int i = 0; i < 5; i++);
}

[slow at kazak ~]$ gcc -std=c99 -Werror=declaration-after-statement -c
test.c

[slow at kazak ~]$ gcc -std=gnu90 -c test.c
test.c: In function ‘main’:
test.c:3:2: error: ‘for’ loop initial declarations are only allowed in
C99 or C11 mode
  for (int i = 0; i < 5; i++);
  ^
test.c:3:2: note: use option -std=c99, -std=gnu99, -std=c11 or
-std=gnu11 to compile your code

So as we can't really catch for loop initial declarations with the
compiler, we should either document that we don't want them in our
coding standard or apply my patch (with an updated commit message) to
ensure the compiler accepts it.

> > So I guess cherry-picking is the wrong approach. It should be all or
> > nothing.
> > 
> > Attached is a patch that would continue cherry-picking C99 features
> > and adds a check for mixed declarations and code, but this is just
> > meant to illustrate the problem.
> > 
> > Maybe we should take this further and be as pedantic as autoconf's
> > AC_PROG_CC_C99:
> > 
> > # If the C compiler is not in ISO C99 mode by default, try to add an
> > # option to output variable CC to make it so.  This macro tries
> > # various options that select ISO C99 on some system or another.  It
> > # considers the compiler to be in ISO C99 mode if it handles _Bool,
> > # // comments, flexible array members, inline, long long int, mixed
> > # code and declarations, named initialization of structs, restrict,
> > # va_copy, varargs macros, variable declarations in for loops and
> > # variable length arrays.
> 
> I'd really try to avoid // comments,

agreed

> flexible array members,

as Volker points out, we already use this and for good reason imo.

> mixed code and declarations

arguable. And then, it seems the right place to enforce this is our
coding standard and not cherry-picking C standard features and
compiler warnings/errors.

> and variable declarations in for loops.

this is the best place to declare them if not used in the outer scope.

Updated patch attached. :)

Cheerio!
-slow
-------------- next part --------------
From 96743144d496fd4a676246fb1a541b908e54c585 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 27 Apr 2016 16:05:29 +0200
Subject: [PATCH] waf: correct C99 detection for older GCC with a default of
 -std=gnu90
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Unfortunately -Werror=declaration-after-statement doesn't catch for loop
variable declarations:

$ cat test.c
int main(int argc, char **argv) {
        for (int i = 0; i < 5; i++);
}

$ gcc -std=c99 -Werror=declaration-after-statement -c test.c
$

Older GCC compilers have a default C standard of gnu90. That includes
C90, some GNU extensions and some parts of C99, including designated
initializers, but *not* intermingled code and variable declarations. As
a result, the same compilation fails:

$ gcc -std=gnu90 -c test.c
test.c: In function ‘main’:
test.c:3:2: error: ‘for’ loop initial declarations are only allowed in
C99 or C11 mode

  for (int i = 0; i < 5; i++);
  ^
test.c:3:2: note: use option -std=c99, -std=gnu99, -std=c11 or
-std=gnu11 to compile your code

To fix this, enhance our check which compiler flags are needed for C99
mode by adding code to the waf check that uses intermingled code and
variable declarations.

If we want to discourage use of for loop variable declarations, we
should do this in our coding standard.

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 buildtools/wafsamba/wscript | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/buildtools/wafsamba/wscript b/buildtools/wafsamba/wscript
index 8802e5a..08704d1 100755
--- a/buildtools/wafsamba/wscript
+++ b/buildtools/wafsamba/wscript
@@ -465,11 +465,14 @@ def configure(conf):
         conf.DEFINE('_BSD_TYPES', 1, add_to_cflags=True)
 
     # Try to find the right extra flags for C99 initialisers
-    for f in ["", "-AC99", "-qlanglvl=extc99", "-qlanglvl=stdc99", "-c99"]:
-        if conf.CHECK_CFLAGS([f], '''
-struct foo {int x;char y;};
-struct foo bar = { .y = 'X', .x = 1 };
-'''):
+    # and C99 intermingled declarations and code
+    for f in ["", "-std=c99", "-AC99", "-qlanglvl=extc99", "-qlanglvl=stdc99", "-c99"]:
+        if conf.CHECK_CODE('''
+            struct foo {int x;char y;};
+            struct foo bar = { .y = 'X', .x = 1 };
+            for (int i=0; i<2; i++) { bar.x+=i; }
+            ''',
+            define='HAVE_C99', cflags=f, addmain=True):
             if f != "":
                 conf.ADD_CFLAGS(f)
             break
-- 
2.5.0