diff mbox

Make C++ use the enum mode attribute

Message ID HE1PR07MB0905CBC2F8154F44BB32E14AE4D30@HE1PR07MB0905.eurprd07.prod.outlook.com
State New
Headers show

Commit Message

Bernd Edlinger Feb. 6, 2016, 8:36 p.m. UTC
Hi Jakub,

as a follow-up for your c/69669 fix, I'd like to have the enum mode also honored
in C++ code, because the mode attribute now finally really works in C, but it is
completely and silently(!) ignored by C++ code, which results in incompatible
code.

So I duplicated what is done in c/c-decl.c also in cp/decl.c.  That worked
immediately, except that it is not possible to explicitly check the "mode"
attribute in the TYPE_ATTRIBUTES (enumtype) because that attribute
is never copied to the type, and the original attribute list is not available
here.  That should be OK, as this check was added to fix pr52085 which
does not apply here, because C++ does not support enum forward
declarations.

If that patch is not appropriate for stage  4, I would at least want to emit
an attribute directive ignored warning in c++ mode.  I think that could
be done in handle_mode_attribute.  But fixing that feature is cooler.

Boot-strapped and regression tested on x86_64-pc-linux-gnu.
Ok for trunk?


Thanks
Bernd.
cp:
2016-02-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* decl.c (finish_enum_value_list): Use the specified mode.

testsuite:
2016-02-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/pr69669.c: Check the used mode.

Comments

Mike Stump Feb. 6, 2016, 9:13 p.m. UTC | #1
On Feb 6, 2016, at 12:36 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> If that patch is not appropriate for stage  4

Without looking at the patch, fixes to abi incompatibilities I think are important.  Would be nice to fix them.

[ ok, I peeked at the patch ]

Wow, nice, short and sweet.  I’d like for it to be considered.
Trevor Saunders Feb. 6, 2016, 10:37 p.m. UTC | #2
On Sat, Feb 06, 2016 at 08:36:27PM +0000, Bernd Edlinger wrote:
> 
> Hi Jakub,
> 
> as a follow-up for your c/69669 fix, I'd like to have the enum mode also honored
> in C++ code, because the mode attribute now finally really works in C, but it is
> completely and silently(!) ignored by C++ code, which results in incompatible
> code.
> 
> So I duplicated what is done in c/c-decl.c also in cp/decl.c.  That worked
> immediately, except that it is not possible to explicitly check the "mode"
> attribute in the TYPE_ATTRIBUTES (enumtype) because that attribute
> is never copied to the type, and the original attribute list is not available
> here.  That should be OK, as this check was added to fix pr52085 which
> does not apply here, because C++ does not support enum forward
> declarations.

it is allowed if you do something like

enum X : int;

but it seems really pointless to support setting the size of something
when the language gives you a standard way to do that.

> If that patch is not appropriate for stage  4, I would at least want to emit
> an attribute directive ignored warning in c++ mode.  I think that could
> be done in handle_mode_attribute.  But fixing that feature is cooler.

 Its definitely bad to sighlently ignore the attribute.  On one hand
 being compatable with C seems nice, but on the other hand in C++ 11 and
 later you can set the size without using attributes, and the mode thing
 has always seemed a bit odd to me since its not really clear what C
 type they corespond to.

 Trev

> 
> Boot-strapped and regression tested on x86_64-pc-linux-gnu.
> Ok for trunk?
> 
> 
> Thanks
> Bernd.

> cp:
> 2016-02-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* decl.c (finish_enum_value_list): Use the specified mode.
> 
> testsuite:
> 2016-02-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-c++-common/pr69669.c: Check the used mode.

> Index: gcc/cp/decl.c
> ===================================================================
> --- gcc/cp/decl.c	(Revision 233176)
> +++ gcc/cp/decl.c	(Arbeitskopie)
> @@ -13310,6 +13310,19 @@ finish_enum_value_list (tree enumtype)
>        use_short_enum = flag_short_enums
>          || lookup_attribute ("packed", TYPE_ATTRIBUTES (enumtype));
>  
> +      /* If the precision of the type was specified with an attribute and it
> +	 was too small, give an error.  Otherwise, use it.  */
> +      if (TYPE_PRECISION (enumtype))
> +	{
> +	  if (precision > TYPE_PRECISION (enumtype))
> +	    error ("specified mode too small for enumeral values");
> +	  else
> +	    {
> +	      use_short_enum = true;
> +	      precision = TYPE_PRECISION (enumtype);
> +	    }
> +	}
> +
>        for (itk = (use_short_enum ? itk_char : itk_int);
>             itk != itk_none;
>             itk++)
> Index: gcc/testsuite/c-c++-common/pr69669.c
> ===================================================================
> --- gcc/testsuite/c-c++-common/pr69669.c	(Revision 233176)
> +++ gcc/testsuite/c-c++-common/pr69669.c	(Arbeitskopie)
> @@ -1,5 +1,6 @@
>  /* PR c/69669 */
>  /* { dg-do compile } */
> +/* { dg-options "-fdump-rtl-final" } */
>  
>  enum __attribute__((mode(QI))) E { F = 1 };
>  
> @@ -8,3 +9,5 @@ foo (enum E *x, int y)
>  {
>    *x = (enum E) y;
>  }
> +
> +/* { dg-final { scan-rtl-dump-times "mem:QI" 1 "final" } } */
Mike Stump Feb. 8, 2016, 9:41 p.m. UTC | #3
On Feb 6, 2016, at 2:37 PM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:
> it is allowed if you do something like
> 
> enum X : int;
> 
> but it seems really pointless to support setting the size of something
> when the language gives you a standard way to do that.

Yes, it is, as long as you never use header files, and multiple languages.  :-)  Unfortunately, those are vended features of C and C++.  Once you have a C project that you want to be able to be used from C++, it then is handy to be able to do this.
Trevor Saunders Feb. 9, 2016, 12:27 a.m. UTC | #4
On Mon, Feb 08, 2016 at 01:41:03PM -0800, Mike Stump wrote:
> On Feb 6, 2016, at 2:37 PM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:
> > it is allowed if you do something like
> > 
> > enum X : int;
> > 
> > but it seems really pointless to support setting the size of something
> > when the language gives you a standard way to do that.
> 
> Yes, it is, as long as you never use header files, and multiple languages.  :-)  Unfortunately, those are vended features of C and C++.  Once you have a C project that you want to be able to be used from C++, it then is handy to be able to do this.

Sure, but how many people use the mode attribute on enums?  Probably more than
one, and I guess it doesn't really hurt that much so whatever.

and yes I admitt I'm a little biased against this attribute in the first
place ;)

Trev

>
diff mbox

Patch

Index: gcc/cp/decl.c
===================================================================
--- gcc/cp/decl.c	(Revision 233176)
+++ gcc/cp/decl.c	(Arbeitskopie)
@@ -13310,6 +13310,19 @@  finish_enum_value_list (tree enumtype)
       use_short_enum = flag_short_enums
         || lookup_attribute ("packed", TYPE_ATTRIBUTES (enumtype));
 
+      /* If the precision of the type was specified with an attribute and it
+	 was too small, give an error.  Otherwise, use it.  */
+      if (TYPE_PRECISION (enumtype))
+	{
+	  if (precision > TYPE_PRECISION (enumtype))
+	    error ("specified mode too small for enumeral values");
+	  else
+	    {
+	      use_short_enum = true;
+	      precision = TYPE_PRECISION (enumtype);
+	    }
+	}
+
       for (itk = (use_short_enum ? itk_char : itk_int);
            itk != itk_none;
            itk++)
Index: gcc/testsuite/c-c++-common/pr69669.c
===================================================================
--- gcc/testsuite/c-c++-common/pr69669.c	(Revision 233176)
+++ gcc/testsuite/c-c++-common/pr69669.c	(Arbeitskopie)
@@ -1,5 +1,6 @@ 
 /* PR c/69669 */
 /* { dg-do compile } */
+/* { dg-options "-fdump-rtl-final" } */
 
 enum __attribute__((mode(QI))) E { F = 1 };
 
@@ -8,3 +9,5 @@  foo (enum E *x, int y)
 {
   *x = (enum E) y;
 }
+
+/* { dg-final { scan-rtl-dump-times "mem:QI" 1 "final" } } */