diff mbox

[C] PR43651: add warning for duplicate qualifier

Message ID 5708F58A.7090701@gmail.com
State New
Headers show

Commit Message

Mikhail Maltsev April 9, 2016, 12:28 p.m. UTC
On 04/08/2016 08:54 PM, Martin Sebor wrote:
>> 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?
It is an error in C++ mode. Do we want to change this behavior?

> 
> 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.
> 
Fixed.

Comments

Martin Sebor April 10, 2016, 8:12 p.m. UTC | #1
On 04/09/2016 06:28 AM, Mikhail Maltsev wrote:
> On 04/08/2016 08:54 PM, Martin Sebor wrote:
>>> 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?
> It is an error in C++ mode. Do we want to change this behavior?

You're right, G++ does give an error.  I missed it in my testing.
Unlike C11, C++ requires a diagnostic for duplicated cv-qualifiers
so by issuing a warning Clang is more permissive.  My personal
inclination would be to treat this consistently between C and C++
but whether or not to change it is something Jason would need to
weigh in on.

>> 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.
>>
> Fixed.

Great, thank you.

Martin
Mikhail Maltsev April 28, 2016, noon UTC | #2
On 04/10/2016 11:12 PM, Martin Sebor wrote:
> On 04/09/2016 06:28 AM, Mikhail Maltsev wrote:
>> On 04/08/2016 08:54 PM, Martin Sebor wrote:
>>>> 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?
>> It is an error in C++ mode. Do we want to change this behavior?
> 
> You're right, G++ does give an error.  I missed it in my testing.
> Unlike C11, C++ requires a diagnostic for duplicated cv-qualifiers
> so by issuing a warning Clang is more permissive.  My personal
> inclination would be to treat this consistently between C and C++
> but whether or not to change it is something Jason would need to
> weigh in on.

Ping.
Joseph Myers May 10, 2016, 7:51 p.m. UTC | #3
On Sat, 9 Apr 2016, Mikhail Maltsev wrote:

> gcc/c/ChangeLog:
> 
> 2016-04-08  Mikhail Maltsev  <maltsevm@gmail.com>
> 
>         PR c/43651
>         * c-decl.c (declspecs_add_qual): Warn when -Wduplicate-decl-specifier
>         is enabled.
>         * c-errors.c (pedwarn_c90): Return true if warned.
>         * c-tree.h (pedwarn_c90): Change return type to bool.
>         (enum c_declspec_word): Add new enumerator cdw_atomic.
> 
> gcc/ChangeLog:
> 
> 2016-04-08  Mikhail Maltsev  <maltsevm@gmail.com>
> 
>         PR c/43651
>         * doc/invoke.texi (Wduplicate-decl-specifier): Document new option.
> 
> gcc/testsuite/ChangeLog:
> 
> 2016-04-08  Mikhail Maltsev  <maltsevm@gmail.com>
> 
>         PR c/43651
>         * gcc.dg/Wduplicate-decl-specifier-c11.c: New test.
>         * gcc.dg/Wduplicate-decl-specifier.c: Likewise.
> 
> 
> gcc/c-family/ChangeLog:
> 
> 2016-04-08  Mikhail Maltsev  <maltsevm@gmail.com>
> 
>         PR c/43651
>         * c.opt (Wduplicate-decl-specifier): New option.

OK.
Mikhail Maltsev May 11, 2016, 8:24 p.m. UTC | #4
On 05/10/2016 10:51 PM, Joseph Myers wrote:
> On Sat, 9 Apr 2016, Mikhail Maltsev wrote:
> 
>> gcc/c/ChangeLog:
>>
>> 2016-04-08  Mikhail Maltsev  <maltsevm@gmail.com>
>>
>>         PR c/43651
>>         * c-decl.c (declspecs_add_qual): Warn when -Wduplicate-decl-specifier
>>         is enabled.
>>         * c-errors.c (pedwarn_c90): Return true if warned.
>>         * c-tree.h (pedwarn_c90): Change return type to bool.
>>         (enum c_declspec_word): Add new enumerator cdw_atomic.
>>
>> gcc/ChangeLog:
>>
>> 2016-04-08  Mikhail Maltsev  <maltsevm@gmail.com>
>>
>>         PR c/43651
>>         * doc/invoke.texi (Wduplicate-decl-specifier): Document new option.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2016-04-08  Mikhail Maltsev  <maltsevm@gmail.com>
>>
>>         PR c/43651
>>         * gcc.dg/Wduplicate-decl-specifier-c11.c: New test.
>>         * gcc.dg/Wduplicate-decl-specifier.c: Likewise.
>>
>>
>> gcc/c-family/ChangeLog:
>>
>> 2016-04-08  Mikhail Maltsev  <maltsevm@gmail.com>
>>
>>         PR c/43651
>>         * c.opt (Wduplicate-decl-specifier): New option.
> 
> OK.
> 

Committed as r236142.
diff mbox

Patch

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 4f86876..f7f844d 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 declaration has duplicate const, volatile, restrict or _Atomic 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..bc3af02 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -9539,32 +9539,48 @@  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:
       dupe = specs->atomic_p;
       specs->atomic_p = true;
+      prev_loc = specs->locations[cdw_atomic];
+      specs->locations[cdw_atomic] = loc;
       break;
     default:
       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..a3fad13 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -253,6 +253,7 @@  enum c_declspec_word {
   cdw_const,
   cdw_volatile,
   cdw_restrict,
+  cdw_atomic,
   cdw_saturating,
   cdw_alignas,
   cdw_address_space,
@@ -716,7 +717,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/doc/invoke.texi b/gcc/doc/invoke.texi
index e9763d4..63f21e9 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -3538,6 +3538,7 @@  Options} and @ref{Objective-C and Objective-C++ Dialect Options}.
 -Wc++11-compat  -Wc++14-compat@gol
 -Wchar-subscripts  @gol
 -Wcomment  @gol
+-Wduplicate-decl-specifier @r{(C and Objective-C only)} @gol
 -Wenum-compare @r{(in C/ObjC; this is on by default in C++)} @gol
 -Wformat   @gol
 -Wimplicit @r{(C and Objective-C only)} @gol
@@ -3694,6 +3695,13 @@  float area(float radius)
 the compiler performs the entire computation with @code{double}
 because the floating-point literal is a @code{double}.
 
+@item -Wduplicate-decl-specifier @r{(C and Objective-C only)}
+@opindex Wduplicate-decl-specifier
+@opindex Wno-duplicate-decl-specifier
+Warn if a declaration has duplicate @code{const}, @code{volatile},
+@code{restrict} or @code{_Atomic} specifier.  This warning is enabled by
+@option{-Wall}.
+
 @item -Wformat
 @itemx -Wformat=@var{n}
 @opindex Wformat
diff --git a/gcc/testsuite/gcc.dg/Wduplicate-decl-specifier-c11.c b/gcc/testsuite/gcc.dg/Wduplicate-decl-specifier-c11.c
new file mode 100644
index 0000000..c2db525
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wduplicate-decl-specifier-c11.c
@@ -0,0 +1,27 @@ 
+/* { dg-do compile } */
+/* { dg-options "-std=c11 -Wduplicate-decl-specifier" } */
+
+typedef _Atomic int AT1;
+#define AT2 _Atomic int
+
+void
+foo (void)
+{
+  _Atomic AT1 x1;
+  _Atomic AT2 x2;
+  AT1 _Atomic x3;
+  AT2 _Atomic x4;
+  _Atomic int _Atomic x5; /* { dg-warning "duplicate ._Atomic." } */
+}
+
+void a1(_Atomic AT1 t) { }
+void a2(_Atomic AT2 t) { }
+void a3(AT1 _Atomic t) { }
+void a4(AT2 _Atomic t) { }
+void a5(_Atomic int _Atomic t) { }  /* { dg-warning "duplicate ._Atomic." } */
+
+typedef _Atomic AT1 AAT1;
+typedef _Atomic AT2 AAT2;
+typedef AT1 _Atomic AT1A;
+typedef AT2 _Atomic AT2A;
+typedef _Atomic int _Atomic AIA;    /* { dg-warning "duplicate ._Atomic." } */
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..d7a3919
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wduplicate-decl-specifier.c
@@ -0,0 +1,63 @@ 
+/* 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." } */