* [patch] libstdc++/56158 Extend valid values of iostream bitmask types
@ 2015-11-11 9:48 Jonathan Wakely
2015-11-12 15:48 ` Martin Sebor
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Wakely @ 2015-11-11 9:48 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 631 bytes --]
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?
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?
[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 3590 bytes --]
commit ac2fc08a638c7f01ecb5b9e9b3d3a58caf031534
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Wed Nov 11 09:25:38 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..ba4ef92 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)__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)__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)__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;
}
}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] libstdc++/56158 Extend valid values of iostream bitmask types
2015-11-11 9:48 [patch] libstdc++/56158 Extend valid values of iostream bitmask types Jonathan Wakely
@ 2015-11-12 15:48 ` Martin Sebor
2015-11-12 17:09 ` Jonathan Wakely
0 siblings, 1 reply; 7+ messages in thread
From: Martin Sebor @ 2015-11-12 15:48 UTC (permalink / raw)
To: Jonathan Wakely, libstdc++, gcc-patches
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__
>
> 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__: ;
}
}
Martin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] libstdc++/56158 Extend valid values of iostream bitmask types
2015-11-12 15:48 ` Martin Sebor
@ 2015-11-12 17:09 ` Jonathan Wakely
2015-11-12 18:09 ` Martin Sebor
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Wakely @ 2015-11-12 17:09 UTC (permalink / raw)
To: Martin Sebor; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]
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.
[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 3575 bytes --]
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;
}
}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] libstdc++/56158 Extend valid values of iostream bitmask types
2015-11-12 17:09 ` Jonathan Wakely
@ 2015-11-12 18:09 ` Martin Sebor
2015-11-13 8:39 ` Jonathan Wakely
0 siblings, 1 reply; 7+ messages in thread
From: Martin Sebor @ 2015-11-12 18:09 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
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__: ;
;
}
}
Martin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] libstdc++/56158 Extend valid values of iostream bitmask types
2015-11-12 18:09 ` Martin Sebor
@ 2015-11-13 8:39 ` Jonathan Wakely
2015-11-13 9:11 ` Jonathan Wakely
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Wakely @ 2015-11-13 8:39 UTC (permalink / raw)
To: Martin Sebor; +Cc: libstdc++, gcc-patches
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).
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] libstdc++/56158 Extend valid values of iostream bitmask types
2015-11-13 8:39 ` Jonathan Wakely
@ 2015-11-13 9:11 ` Jonathan Wakely
2015-11-25 13:54 ` Jonathan Wakely
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Wakely @ 2015-11-13 9:11 UTC (permalink / raw)
To: Martin Sebor; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2728 bytes --]
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?
[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 2671 bytes --]
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");
}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] libstdc++/56158 Extend valid values of iostream bitmask types
2015-11-13 9:11 ` Jonathan Wakely
@ 2015-11-25 13:54 ` Jonathan Wakely
0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Wakely @ 2015-11-25 13:54 UTC (permalink / raw)
To: Martin Sebor; +Cc: libstdc++, gcc-patches
On 13/11/15 09:11 +0000, Jonathan Wakely wrote:
>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?
I've committed this to trunk, and am backporting the pr56158 fix (with
these test improvements) to gcc-5-branch.
>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");
> }
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-25 13:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-11 9:48 [patch] libstdc++/56158 Extend valid values of iostream bitmask types Jonathan Wakely
2015-11-12 15:48 ` Martin Sebor
2015-11-12 17:09 ` Jonathan Wakely
2015-11-12 18:09 ` Martin Sebor
2015-11-13 8:39 ` Jonathan Wakely
2015-11-13 9:11 ` Jonathan Wakely
2015-11-25 13:54 ` Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).