diff mbox

[C] PR43651: add warning for duplicate qualifier

Message ID 570241F0.3070705@gmail.com
State New
Headers show

Commit Message

Mikhail Maltsev April 4, 2016, 10:29 a.m. UTC
Hi all!

Currently GCC produces pedantic warning, if variable declaration (or
typedef) has duplicate qualifier, but only when compiling as C89 (not
C99 or C11).

The attached patch adds a new warning option to enable the same warning
in C99 and C11. It also checks whether qualifiers come from macro
expansion, e.g.:

#define CT2 const int
const CT2 x1;
CT2 const x2;

and does not warn in this case, but warns for, e.g.

void foo(const int const *x) { }
(because this probably meant to be "const int *const x")

The name for new option "-Wduplicate-decl-specifier" and wording was
chosen to match the same option in Clang.

Bootstrapped and regtested on x86_64-linux. OK for next stage 1?

Comments

Joseph Myers April 7, 2016, 9:50 p.m. UTC | #1
New options need documenting in invoke.texi.
Martin Sebor April 8, 2016, 5:54 p.m. UTC | #2
On 04/04/2016 04:29 AM, Mikhail Maltsev wrote:
> Hi all!
>
> Currently GCC produces pedantic warning, if variable declaration (or
> typedef) has duplicate qualifier, but only when compiling as C89 (not
> C99 or C11).

Presumably that's because C89 makes duplicating a type qualifier
a constraint violation while C99 and C11 allow them.

>
> The attached patch adds a new warning option to enable the same warning
> in C99 and C11.
> It also checks whether qualifiers come from macro
> expansion, e.g.:
>
> #define CT2 const int
> const CT2 x1;
> CT2 const x2;
>
> and does not warn in this case, but warns for, e.g.
>
> void foo(const int const *x) { }
> (because this probably meant to be "const int *const x")
>
> The name for new option "-Wduplicate-decl-specifier" and wording was
> chosen to match the same option in Clang.

My version of Clang also warns in C++ mode but if I'm reading
the patch right, GCC would warn only C mode.  I would find it
surprising if GCC provided the same option as Clang but didn't
make it available in the same languages.  Do you have some
reason for leaving it out that I'm not thinking of?

Also, in C11 mode, Clang issues the warning for duplicated
_Atomic qualifiers but it doesn't look like GCC would with
the patch.  Here again, unless there's some reason not to,
I would expect GCC to issue the same warning as Clang for
the same code.

Martin
diff mbox

Patch

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 4f86876..1650b25 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1000,6 +1000,10 @@  Wsubobject-linkage
 C++ ObjC++ Var(warn_subobject_linkage) Warning Init(1)
 Warn if a class type has a base or a field whose type uses the anonymous namespace or depends on a type with no linkage.
 
+Wduplicate-decl-specifier
+C ObjC Var(warn_duplicate_decl_specifier) Warning LangEnabledBy(C ObjC,Wall)
+Warn when a variable declaration has duplicate const, volatile or restrict specifier.
+
 ansi
 C ObjC C++ ObjC++
 A synonym for -std=c89 (for C) or -std=c++98 (for C++).
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 0dd2121..9902326 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -9539,21 +9539,25 @@  declspecs_add_qual (source_location loc,
   gcc_assert (TREE_CODE (qual) == IDENTIFIER_NODE
 	      && C_IS_RESERVED_WORD (qual));
   i = C_RID_CODE (qual);
+  location_t prev_loc = 0;
   switch (i)
     {
     case RID_CONST:
       dupe = specs->const_p;
       specs->const_p = true;
+      prev_loc = specs->locations[cdw_const];
       specs->locations[cdw_const] = loc;
       break;
     case RID_VOLATILE:
       dupe = specs->volatile_p;
       specs->volatile_p = true;
+      prev_loc = specs->locations[cdw_volatile];
       specs->locations[cdw_volatile] = loc;
       break;
     case RID_RESTRICT:
       dupe = specs->restrict_p;
       specs->restrict_p = true;
+      prev_loc = specs->locations[cdw_restrict];
       specs->locations[cdw_restrict] = loc;
       break;
     case RID_ATOMIC:
@@ -9564,7 +9568,17 @@  declspecs_add_qual (source_location loc,
       gcc_unreachable ();
     }
   if (dupe)
-    pedwarn_c90 (loc, OPT_Wpedantic, "duplicate %qE", qual);
+    {
+      bool warned = pedwarn_c90 (loc, OPT_Wpedantic,
+				 "duplicate %qE declaration specifier", qual);
+      if (!warned
+	  && warn_duplicate_decl_specifier
+	  && prev_loc >= RESERVED_LOCATION_COUNT
+	  && !from_macro_expansion_at (prev_loc)
+	  && !from_macro_expansion_at (loc))
+	warning_at (loc, OPT_Wduplicate_decl_specifier,
+		    "duplicate %qE declaration specifier", qual);
+    }
   return specs;
 }
 
diff --git a/gcc/c/c-errors.c b/gcc/c/c-errors.c
index d5e78b8..af157c0 100644
--- a/gcc/c/c-errors.c
+++ b/gcc/c/c-errors.c
@@ -71,11 +71,12 @@  pedwarn_c99 (location_t location, int opt, const char *gmsgid, ...)
    ISO C99 but not supported in ISO C90, thus we explicitly don't pedwarn
    when C99 is specified.  (There is no flag_c90.)  */
 
-void
+bool
 pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...)
 {
   diagnostic_info diagnostic;
   va_list ap;
+  bool warned = false;
   rich_location richloc (line_table, location);
 
   va_start (ap, gmsgid);
@@ -92,6 +93,7 @@  pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...)
 			       ? DK_PEDWARN : DK_WARNING);
 	  diagnostic.option_index = opt;
 	  report_diagnostic (&diagnostic);
+	  warned = true;
 	  goto out;
 	}
     }
@@ -114,7 +116,9 @@  pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...)
       diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc, DK_PEDWARN);
       diagnostic.option_index = opt;
       report_diagnostic (&diagnostic);
+      warned = true;
     }
 out:
   va_end (ap);
+  return warned;
 }
diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index 96ab049..ac72820 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -716,7 +716,7 @@  extern void c_bind (location_t, tree, bool);
 extern bool tag_exists_p (enum tree_code, tree);
 
 /* In c-errors.c */
-extern void pedwarn_c90 (location_t, int opt, const char *, ...)
+extern bool pedwarn_c90 (location_t, int opt, const char *, ...)
     ATTRIBUTE_GCC_DIAG(3,4);
 extern bool pedwarn_c99 (location_t, int opt, const char *, ...)
     ATTRIBUTE_GCC_DIAG(3,4);
diff --git a/gcc/testsuite/gcc.dg/Wduplicate-decl-specifier.c b/gcc/testsuite/gcc.dg/Wduplicate-decl-specifier.c
new file mode 100644
index 0000000..6b939fb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wduplicate-decl-specifier.c
@@ -0,0 +1,62 @@ 
+/* PR43651 */
+/* { dg-do compile } */
+/* { dg-options -Wduplicate-decl-specifier } */
+
+typedef const int CT1;
+#define CT2 const int
+typedef volatile int VT1;
+#define VT2 volatile int
+typedef char *restrict RT1;
+#define RT2 char *restrict
+
+void foo(void)
+{
+    const CT1 x1;
+    const CT2 x2;
+    CT1 const x3;
+    CT2 const x4;
+    const int const x5;         /* { dg-warning "duplicate .const." } */
+    const int *const x6;
+    volatile VT1 y1;
+    volatile VT2 y2;
+    VT1 volatile y3;
+    VT2 volatile y4;
+    volatile int volatile y5;   /* { dg-warning "duplicate .volatile." } */
+    volatile int *volatile y6;
+    RT1 restrict r1;
+    RT2 restrict r2;
+    restrict RT1 r3;
+    /* "restrict RT2" is invalid */
+    char *restrict restrict r4; /* { dg-warning "duplicate .restrict." } */
+    char *restrict *restrict r5;
+}
+
+void c1(const CT1 t) { }
+void c2(const CT2 t) { }
+void c3(CT1 const t) { }
+void c4(CT2 const t) { }
+void c5(const int const t) { }  /* { dg-warning "duplicate .const." } */
+void v1(volatile VT1 t) { }
+void v2(volatile VT2 t) { }
+void v3(VT1 volatile t) { }
+void v4(VT2 volatile t) { }
+void v5(volatile int volatile t) { } /* { dg-warning "duplicate .volatile." } */
+void r1(restrict RT1 t) { }
+void r2(RT1 restrict t) { }
+void r3(RT2 restrict t) { }
+void r4(char *restrict restrict t) { }  /* { dg-warning "duplicate .restrict." } */
+
+typedef const CT1 CCT1;
+typedef const CT2 CCT2;
+typedef CT1 const CT1C;
+typedef CT2 const CT2C;
+typedef const int const CIC;    /* { dg-warning "duplicate .const." } */
+typedef volatile VT1 VVT1;
+typedef volatile VT2 VVT2;
+typedef VT1 volatile VT1V;
+typedef VT2 volatile VT2V;
+typedef volatile int volatile VIV; /* { dg-warning "duplicate .volatile." } */
+typedef RT1 restrict RT1R;
+typedef RT2 restrict RT2R;
+typedef restrict RT1 RRT1;
+typedef int *restrict restrict IRR; /* { dg-warning "duplicate .restrict." } */