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

[Jonathan Wakely <jwakely at redhat dot com>]

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

We could use something like this:

 switch(f()) {
 case ~ios::iostate():
 default:
   break;
 }

Which with no warning options at all (or with -Wno-switch to suppress
other warnings) gives this for the buggy code currently on trunk:

e.cc:13:2: warning: case label value is less than minimum value for type
 case ~ios::iostate():
 ^

But that could break if someone fixes that warning to depend on
-Wswitch (which it probably should do). And with -Wswitch that gives a
different warning:

e.cc:10:3: warning: case value â-1â not in enumerated type âEâ [-Wswitch]

So I think I'll just leave the tests as they are, and rely on our
diligent Clang users to report if there's still a problem :-)

I've committed the attached patch to trunk.

commit 2ec414f95401368dc532b3fc6155ed7be41edb8e
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Nov 12 16:16:20 2015 +0000

    Extend valid values of iostream bitmask types
    
    	PR libstdc++/56158
    	* include/bits/ios_base.h (_Ios_Fmtflags, _Ios_Openmode, _Ios_Iostate):
    	Define enumerators to ensure all values of type int are valid values
    	of the enumeration type.
    	* testsuite/27_io/ios_base/types/fmtflags/case_label.cc: Add new cases.
    	* 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/include/bits/ios_base.h b/libstdc++-v3/include/bits/ios_base.h
index 44029ad..908ba7c 100644
--- a/libstdc++-v3/include/bits/ios_base.h
+++ b/libstdc++-v3/include/bits/ios_base.h
@@ -74,7 +74,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _S_adjustfield 	= _S_left | _S_right | _S_internal,
       _S_basefield 	= _S_dec | _S_oct | _S_hex,
       _S_floatfield 	= _S_scientific | _S_fixed,
-      _S_ios_fmtflags_end = 1L << 16 
+      _S_ios_fmtflags_end = 1L << 16,
+      _S_ios_fmtflags_max = __INT_MAX__,
+      _S_ios_fmtflags_min = ~__INT_MAX__
     };
 
   inline _GLIBCXX_CONSTEXPR _Ios_Fmtflags
@@ -114,7 +116,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _S_in 		= 1L << 3,
       _S_out 		= 1L << 4,
       _S_trunc 		= 1L << 5,
-      _S_ios_openmode_end = 1L << 16 
+      _S_ios_openmode_end = 1L << 16,
+      _S_ios_openmode_max = __INT_MAX__,
+      _S_ios_openmode_min = ~__INT_MAX__
     };
 
   inline _GLIBCXX_CONSTEXPR _Ios_Openmode
@@ -152,7 +156,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _S_badbit 		= 1L << 0,
       _S_eofbit 		= 1L << 1,
       _S_failbit		= 1L << 2,
-      _S_ios_iostate_end = 1L << 16 
+      _S_ios_iostate_end = 1L << 16,
+      _S_ios_iostate_max = __INT_MAX__,
+      _S_ios_iostate_min = ~__INT_MAX__
     };
 
   inline _GLIBCXX_CONSTEXPR _Ios_Iostate
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 591e371..e8820c5 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,5 +70,9 @@ case_labels(bitmask_type b)
       break;
     case std::_S_ios_fmtflags_end:
       break;
+    case std::_S_ios_fmtflags_min:
+      break;
+    case std::_S_ios_fmtflags_max:
+      break;
     }
 }
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 44fb44e..4e4e4f5 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,5 +42,9 @@ case_labels(bitmask_type b)
       break;
     case std::_S_ios_iostate_end:
       break;
+    case std::_S_ios_iostate_min:
+      break;
+    case std::_S_ios_iostate_max:
+      break;
     }
 }
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 267f8a2..8c6672f6 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,5 +46,9 @@ case_labels(bitmask_type b)
       break;
     case std::_S_ios_openmode_end:
       break;
+    case std::_S_ios_openmode_min:
+      break;
+    case std::_S_ios_openmode_max:
+      break;
     }
 }