Re: [patch] libstdc++/56158 Extend valid values of iostream bitmask types

[Jonathan Wakely <jwakely at redhat dot com>]

On 13/11/15 08:39 +0000, Jonathan Wakely wrote:
> On 12/11/15 11:09 -0700, Martin Sebor wrote:
> > On 11/12/2015 10:08 AM, Jonathan Wakely wrote:
> > > On 12/11/15 08:48 -0700, Martin Sebor wrote:
> > > > On 11/11/2015 02:48 AM, Jonathan Wakely wrote:
> > > > > As described in the PR, we have operator~ overloads defined for
> > > > > enumeration types which produce values outside the range of valid
> > > > > values for the type. In C++11 that can be trivially solved by giving
> > > > > the enumeration types a fixed underlying type, but this code needs to
> > > > > be valid in C++03 too.
> > > > >
> > > > > This patch defines new min/max enumerators as INT_MIN/INT_MAX so that
> > > > > every int value is also a valid value for the bitmask type.
> > > > >
> > > > > Does anyone see any problems with this solution, or better solutions?
> > > >
> > > > Just a minor nit that the C-style cast in the below triggers
> > > > a -Wold-style-cast warning in Clang, in case libstdc++ tries
> > > > to be Clang-warning free. Since the type of __INT_MAX__ is
> > > > int it shouldn't be necessary.
> > > >
> > > > +      _S_ios_fmtflags_min = ~(int)__INT_MAX__
> > >
> > > That's worth fixing, thanks.
> > >
> > > > > Any suggestions for how to test this, given that GCC's ubsan doesn't
> > > > > check for this, and we can't run the testsuite with ubsan anyway?
> > > >
> > > > Use a case/switch statement with -Werror=switch-enum to make sure
> > > > all the cases are handled and none is duplicated or outside of the
> > > > valid values of the enumeration:
> > > >
> > > > void foo (ios::iostate s) {
> > > >    switch (s) {
> > > >    case badbit:
> > > >    case eofbit:
> > > >    case failbit:
> > > >    case goodbit:
> > > >    case __INT_MAX__:
> > > >    case ~__INT_MAX__: ;
> > > >  }
> > > > }
> > >
> > > I thought this was a great idea at first ... but -Wswitch-enum will
> > > complain that the end, min and max enumerators are not handled (even
> > > though __INT_MAX__ and ~__INT_MAX__ have the same values as the max
> > > and min ones, respectively).
> >
> > Hmm, I didn't see any warnings for the small test case I wrote and
> > still don't. Just out of curiosity, what did I miss?
> >
> > enum iostate {
> >   goodbit = 0,
> >   eofbit,
> >   failbit,
> >   badbit,
> >   max = __INT_MAX__,
> >   min = ~__INT_MAX__
> > };
> >
> > void foo (iostate s) {
> >   switch (s) {
> >   case badbit:
> >   case eofbit:
> >   case failbit:
> >   case goodbit:
> >   case __INT_MAX__:
> >   case ~__INT_MAX__: ;
> >   ;
> >   }
> > }
>
> I must have messed up my test, I agree this is warning clean.
>
> So I could add tests for those minimum and maximum values, and also
> static_assert that the enumerations have an underlying type with the
> same representation as int (because if it's actually bigger than int
> the operator~ could still produce values outside the range of valid
> values).

How's this?



commit c9fc6b83d0ec4bbf73d7d0f410e0b0e2799d8adf
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Nov 13 08:56:22 2015 +0000

    Improve tests for valid values of iostream bitmask types
    
    	* testsuite/27_io/ios_base/types/fmtflags/case_label.cc: Explicitly
    	check minimum and maximum values, and size of underlying type.
    	* testsuite/27_io/ios_base/types/iostate/case_label.cc: Likewise.
    	* testsuite/27_io/ios_base/types/openmode/case_label.cc: Likewise.

diff --git a/libstdc++-v3/testsuite/27_io/ios_base/types/fmtflags/case_label.cc b/libstdc++-v3/testsuite/27_io/ios_base/types/fmtflags/case_label.cc
index e8820c5..3475fd3 100644
--- a/libstdc++-v3/testsuite/27_io/ios_base/types/fmtflags/case_label.cc
+++ b/libstdc++-v3/testsuite/27_io/ios_base/types/fmtflags/case_label.cc
@@ -70,9 +70,11 @@ case_labels(bitmask_type b)
       break;
     case std::_S_ios_fmtflags_end:
       break;
-    case std::_S_ios_fmtflags_min:
+    case __INT_MAX__:
       break;
-    case std::_S_ios_fmtflags_max:
+    case ~__INT_MAX__:
       break;
     }
+  static_assert( sizeof(std::underlying_type_t<bitmask_type>) == sizeof(int),
+      "underlying type has same range of values as int");
 }
diff --git a/libstdc++-v3/testsuite/27_io/ios_base/types/iostate/case_label.cc b/libstdc++-v3/testsuite/27_io/ios_base/types/iostate/case_label.cc
index 4e4e4f5..a72a774 100644
--- a/libstdc++-v3/testsuite/27_io/ios_base/types/iostate/case_label.cc
+++ b/libstdc++-v3/testsuite/27_io/ios_base/types/iostate/case_label.cc
@@ -42,9 +42,11 @@ case_labels(bitmask_type b)
       break;
     case std::_S_ios_iostate_end:
       break;
-    case std::_S_ios_iostate_min:
+    case __INT_MAX__:
       break;
-    case std::_S_ios_iostate_max:
+    case ~__INT_MAX__:
       break;
     }
+  static_assert( sizeof(std::underlying_type_t<bitmask_type>) == sizeof(int),
+      "underlying type has same range of values as int");
 }
diff --git a/libstdc++-v3/testsuite/27_io/ios_base/types/openmode/case_label.cc b/libstdc++-v3/testsuite/27_io/ios_base/types/openmode/case_label.cc
index 8c6672f6..f621d21 100644
--- a/libstdc++-v3/testsuite/27_io/ios_base/types/openmode/case_label.cc
+++ b/libstdc++-v3/testsuite/27_io/ios_base/types/openmode/case_label.cc
@@ -46,9 +46,11 @@ case_labels(bitmask_type b)
       break;
     case std::_S_ios_openmode_end:
       break;
-    case std::_S_ios_openmode_min:
+    case __INT_MAX__:
       break;
-    case std::_S_ios_openmode_max:
+    case ~__INT_MAX__:
       break;
     }
+  static_assert( sizeof(std::underlying_type_t<bitmask_type>) == sizeof(int),
+      "underlying type has same range of values as int");
 }