You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stdcxx.apache.org by Eric Lemings <Er...@roguewave.com> on 2008/06/02 21:55:51 UTC
[STDCXX-550] Please peer review this change.
Would like to get another set of eyes on this before I submit.
$ svn diff include/deque
Index: include/deque
===================================================================
--- include/deque (revision 662487)
+++ include/deque (working copy)
@@ -749,7 +749,7 @@
void _C_insert (const iterator &__it,
_IntType __n, _IntType __x, int) {
// see 23.1.1, p9 and DR 438
- _C_insert_n (__it, __n, __x);
+ _C_insert_n (__it, __n, const_reference (__x));
}
Thanks,
Brad.
Re: [STDCXX-550] Please peer review this change.
Posted by Martin Sebor <se...@roguewave.com>.
Eric Lemings wrote:
>
>
>> -----Original Message-----
>> From: Martin Sebor [mailto:msebor@gmail.com] On Behalf Of Martin Sebor
>> Sent: Monday, June 02, 2008 5:41 PM
>> To: dev@stdcxx.apache.org
>> Subject: Re: [STDCXX-550] Please peer review this change.
>>
>> Eric Lemings wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Martin Sebor [mailto:msebor@gmail.com] On Behalf Of
>> Martin Sebor
>>>> Sent: Monday, June 02, 2008 4:21 PM
>>>> To: dev@stdcxx.apache.org
>>>> Subject: Re: [STDCXX-550] Please peer review this change.
>>>>
>>>> Eric Lemings wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Martin Sebor [mailto:sebor@roguewave.com]
>>>>>> Sent: Monday, June 02, 2008 2:44 PM
>>>>>> To: dev@stdcxx.apache.org
>>>>>> Subject: Re: [STDCXX-550] Please peer review this change.
>>>>>>
>>>>>> Eric Lemings wrote:
>>>>>>>
>>>>>>> Would like to get another set of eyes on this before I submit.
>>>>>>>
>>>>>>> $ svn diff include/deque
>>>>>>> Index: include/deque
>>>>>>>
>>>> ===================================================================
>>>>>>> --- include/deque (revision 662487)
>>>>>>> +++ include/deque (working copy)
>>>>>>> @@ -749,7 +749,7 @@
>>>>>>> void _C_insert (const iterator &__it,
>>>>>>> _IntType __n, _IntType __x, int) {
>>>>>>> // see 23.1.1, p9 and DR 438
>>>>>>> - _C_insert_n (__it, __n, __x);
>>>>>>> + _C_insert_n (__it, __n, const_reference (__x));
>>>>>> I suspect this is not correct. Suppose the type of __x is
>>>>>> a 4 byte int and const_reference is an 8 byte long. The cast
>>>>>> will make _C_insert_n() to read 4 bytes past the end of __x.
>>>>>> A better fix might be to cast __x to value_type, as long as
>>>>>> doing so doesn't violate [sequence.reqmts].
>>>>> Yep. Good call. Will change to value_type() cast.
>>>> Do read DR 438 before applying the cast. There are some obscure
>>>> corner cases here, e.g., IntType being a user-defined "integer-
>>>> like" type. I don't know if we support this case now, or if we
>>>> do, if it's being tested, but suffice it to say that there are
>>>> subtle differences between direct-initialization either via
>>>> a function-style cast or static_cast, or copy-initialization
>>>> (passing arguments to functions).
>>> I did read the DR briefly. Perhaps I should add some conditional
>>> compilation so that __x is converted using value_type() only when
>>> using Sun C++? That would limit the scope of the change and the
>>> issue only relates to this compiler anywho.
>> I don't think we want to do it differently for different compilers.
>>
>>> Or should I just skip it entirely and leave the warnings as they
>>> are?
>> That depends on what a small test case looks like. How likely are
>> users to run into the same warning and what can they do to silence
>> it? A small test case reproducing the warning should help answer
>> these questions.
>
> I don't think a test case is really needed. Users can easily encounter
> the warnings anytime they use a deque instantiated with 64-bit integer
> types as shown by lines 593-601 in file
> tests/containers/23.deque.modifiers.cpp where the warnings originally
> appeared.
I saw the test and the warnings. It's not obvious from the test what
exactly is being instantiated on what type, and there are 82 warnings
in the build logs. It would be a lot easier to talk about if it was
distilled to a few lines of code.
For example, does this produce a warning:
std::deque<int> d;
long x = LONG_MAX;
d.insert (d.begin (), &x, &x);
or does this:
std::deque<long> d;
int x = INT_MAX;
d.insert (d.begin (), &x, &x);
and/or something else...?
>
> It is just a warning and only for Sun C++ but I see how the explicit
> conversion could potentially cause problems. Not likely but possible.
> I'll just put it on hold for now.
Well, the first case above would lose the high bits of x. Seems like
the warning at the POI would be justified. In the second case I don't
see a need for a warning.
Martin
RE: [STDCXX-550] Please peer review this change.
Posted by Eric Lemings <Er...@roguewave.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:msebor@gmail.com] On Behalf Of Martin Sebor
> Sent: Monday, June 02, 2008 5:41 PM
> To: dev@stdcxx.apache.org
> Subject: Re: [STDCXX-550] Please peer review this change.
>
> Eric Lemings wrote:
> >
> >
> >> -----Original Message-----
> >> From: Martin Sebor [mailto:msebor@gmail.com] On Behalf Of
> Martin Sebor
> >> Sent: Monday, June 02, 2008 4:21 PM
> >> To: dev@stdcxx.apache.org
> >> Subject: Re: [STDCXX-550] Please peer review this change.
> >>
> >> Eric Lemings wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Martin Sebor [mailto:sebor@roguewave.com]
> >>>> Sent: Monday, June 02, 2008 2:44 PM
> >>>> To: dev@stdcxx.apache.org
> >>>> Subject: Re: [STDCXX-550] Please peer review this change.
> >>>>
> >>>> Eric Lemings wrote:
> >>>>>
> >>>>> Would like to get another set of eyes on this before I submit.
> >>>>>
> >>>>> $ svn diff include/deque
> >>>>> Index: include/deque
> >>>>>
> >> ===================================================================
> >>>>> --- include/deque (revision 662487)
> >>>>> +++ include/deque (working copy)
> >>>>> @@ -749,7 +749,7 @@
> >>>>> void _C_insert (const iterator &__it,
> >>>>> _IntType __n, _IntType __x, int) {
> >>>>> // see 23.1.1, p9 and DR 438
> >>>>> - _C_insert_n (__it, __n, __x);
> >>>>> + _C_insert_n (__it, __n, const_reference (__x));
> >>>> I suspect this is not correct. Suppose the type of __x is
> >>>> a 4 byte int and const_reference is an 8 byte long. The cast
> >>>> will make _C_insert_n() to read 4 bytes past the end of __x.
> >>>> A better fix might be to cast __x to value_type, as long as
> >>>> doing so doesn't violate [sequence.reqmts].
> >>> Yep. Good call. Will change to value_type() cast.
> >> Do read DR 438 before applying the cast. There are some obscure
> >> corner cases here, e.g., IntType being a user-defined "integer-
> >> like" type. I don't know if we support this case now, or if we
> >> do, if it's being tested, but suffice it to say that there are
> >> subtle differences between direct-initialization either via
> >> a function-style cast or static_cast, or copy-initialization
> >> (passing arguments to functions).
> >
> > I did read the DR briefly. Perhaps I should add some conditional
> > compilation so that __x is converted using value_type() only when
> > using Sun C++? That would limit the scope of the change and the
> > issue only relates to this compiler anywho.
>
> I don't think we want to do it differently for different compilers.
>
> >
> > Or should I just skip it entirely and leave the warnings as they
> > are?
>
> That depends on what a small test case looks like. How likely are
> users to run into the same warning and what can they do to silence
> it? A small test case reproducing the warning should help answer
> these questions.
I don't think a test case is really needed. Users can easily encounter
the warnings anytime they use a deque instantiated with 64-bit integer
types as shown by lines 593-601 in file
tests/containers/23.deque.modifiers.cpp where the warnings originally
appeared.
It is just a warning and only for Sun C++ but I see how the explicit
conversion could potentially cause problems. Not likely but possible.
I'll just put it on hold for now.
Brad.
Re: [STDCXX-550] Please peer review this change.
Posted by Martin Sebor <se...@roguewave.com>.
Eric Lemings wrote:
>
>
>> -----Original Message-----
>> From: Martin Sebor [mailto:msebor@gmail.com] On Behalf Of Martin Sebor
>> Sent: Monday, June 02, 2008 4:21 PM
>> To: dev@stdcxx.apache.org
>> Subject: Re: [STDCXX-550] Please peer review this change.
>>
>> Eric Lemings wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Martin Sebor [mailto:sebor@roguewave.com]
>>>> Sent: Monday, June 02, 2008 2:44 PM
>>>> To: dev@stdcxx.apache.org
>>>> Subject: Re: [STDCXX-550] Please peer review this change.
>>>>
>>>> Eric Lemings wrote:
>>>>>
>>>>> Would like to get another set of eyes on this before I submit.
>>>>>
>>>>> $ svn diff include/deque
>>>>> Index: include/deque
>>>>>
>> ===================================================================
>>>>> --- include/deque (revision 662487)
>>>>> +++ include/deque (working copy)
>>>>> @@ -749,7 +749,7 @@
>>>>> void _C_insert (const iterator &__it,
>>>>> _IntType __n, _IntType __x, int) {
>>>>> // see 23.1.1, p9 and DR 438
>>>>> - _C_insert_n (__it, __n, __x);
>>>>> + _C_insert_n (__it, __n, const_reference (__x));
>>>> I suspect this is not correct. Suppose the type of __x is
>>>> a 4 byte int and const_reference is an 8 byte long. The cast
>>>> will make _C_insert_n() to read 4 bytes past the end of __x.
>>>> A better fix might be to cast __x to value_type, as long as
>>>> doing so doesn't violate [sequence.reqmts].
>>> Yep. Good call. Will change to value_type() cast.
>> Do read DR 438 before applying the cast. There are some obscure
>> corner cases here, e.g., IntType being a user-defined "integer-
>> like" type. I don't know if we support this case now, or if we
>> do, if it's being tested, but suffice it to say that there are
>> subtle differences between direct-initialization either via
>> a function-style cast or static_cast, or copy-initialization
>> (passing arguments to functions).
>
> I did read the DR briefly. Perhaps I should add some conditional
> compilation so that __x is converted using value_type() only when
> using Sun C++? That would limit the scope of the change and the
> issue only relates to this compiler anywho.
I don't think we want to do it differently for different compilers.
>
> Or should I just skip it entirely and leave the warnings as they
> are?
That depends on what a small test case looks like. How likely are
users to run into the same warning and what can they do to silence
it? A small test case reproducing the warning should help answer
these questions.
Martin
RE: [STDCXX-550] Please peer review this change.
Posted by Eric Lemings <Er...@roguewave.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:msebor@gmail.com] On Behalf Of Martin Sebor
> Sent: Monday, June 02, 2008 4:21 PM
> To: dev@stdcxx.apache.org
> Subject: Re: [STDCXX-550] Please peer review this change.
>
> Eric Lemings wrote:
> >
> >
> >> -----Original Message-----
> >> From: Martin Sebor [mailto:sebor@roguewave.com]
> >> Sent: Monday, June 02, 2008 2:44 PM
> >> To: dev@stdcxx.apache.org
> >> Subject: Re: [STDCXX-550] Please peer review this change.
> >>
> >> Eric Lemings wrote:
> >>>
> >>> Would like to get another set of eyes on this before I submit.
> >>>
> >>> $ svn diff include/deque
> >>> Index: include/deque
> >>>
> ===================================================================
> >>> --- include/deque (revision 662487)
> >>> +++ include/deque (working copy)
> >>> @@ -749,7 +749,7 @@
> >>> void _C_insert (const iterator &__it,
> >>> _IntType __n, _IntType __x, int) {
> >>> // see 23.1.1, p9 and DR 438
> >>> - _C_insert_n (__it, __n, __x);
> >>> + _C_insert_n (__it, __n, const_reference (__x));
> >> I suspect this is not correct. Suppose the type of __x is
> >> a 4 byte int and const_reference is an 8 byte long. The cast
> >> will make _C_insert_n() to read 4 bytes past the end of __x.
> >> A better fix might be to cast __x to value_type, as long as
> >> doing so doesn't violate [sequence.reqmts].
> >
> > Yep. Good call. Will change to value_type() cast.
>
> Do read DR 438 before applying the cast. There are some obscure
> corner cases here, e.g., IntType being a user-defined "integer-
> like" type. I don't know if we support this case now, or if we
> do, if it's being tested, but suffice it to say that there are
> subtle differences between direct-initialization either via
> a function-style cast or static_cast, or copy-initialization
> (passing arguments to functions).
I did read the DR briefly. Perhaps I should add some conditional
compilation so that __x is converted using value_type() only when
using Sun C++? That would limit the scope of the change and the
issue only relates to this compiler anywho.
Or should I just skip it entirely and leave the warnings as they
are?
Brad.
Re: [STDCXX-550] Please peer review this change.
Posted by Martin Sebor <se...@roguewave.com>.
Eric Lemings wrote:
>
>
>> -----Original Message-----
>> From: Martin Sebor [mailto:sebor@roguewave.com]
>> Sent: Monday, June 02, 2008 2:44 PM
>> To: dev@stdcxx.apache.org
>> Subject: Re: [STDCXX-550] Please peer review this change.
>>
>> Eric Lemings wrote:
>>>
>>> Would like to get another set of eyes on this before I submit.
>>>
>>> $ svn diff include/deque
>>> Index: include/deque
>>> ===================================================================
>>> --- include/deque (revision 662487)
>>> +++ include/deque (working copy)
>>> @@ -749,7 +749,7 @@
>>> void _C_insert (const iterator &__it,
>>> _IntType __n, _IntType __x, int) {
>>> // see 23.1.1, p9 and DR 438
>>> - _C_insert_n (__it, __n, __x);
>>> + _C_insert_n (__it, __n, const_reference (__x));
>> I suspect this is not correct. Suppose the type of __x is
>> a 4 byte int and const_reference is an 8 byte long. The cast
>> will make _C_insert_n() to read 4 bytes past the end of __x.
>> A better fix might be to cast __x to value_type, as long as
>> doing so doesn't violate [sequence.reqmts].
>
> Yep. Good call. Will change to value_type() cast.
Do read DR 438 before applying the cast. There are some obscure
corner cases here, e.g., IntType being a user-defined "integer-
like" type. I don't know if we support this case now, or if we
do, if it's being tested, but suffice it to say that there are
subtle differences between direct-initialization either via
a function-style cast or static_cast, or copy-initialization
(passing arguments to functions).
Martin
>
> Brad.
>
>> See also LWG DR 438 for some background:
>> http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#438
>>
>> Martin
>>
RE: [STDCXX-550] Please peer review this change.
Posted by Eric Lemings <Er...@roguewave.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Monday, June 02, 2008 2:44 PM
> To: dev@stdcxx.apache.org
> Subject: Re: [STDCXX-550] Please peer review this change.
>
> Eric Lemings wrote:
> >
> > Would like to get another set of eyes on this before I submit.
> >
> > $ svn diff include/deque
> > Index: include/deque
> > ===================================================================
> > --- include/deque (revision 662487)
> > +++ include/deque (working copy)
> > @@ -749,7 +749,7 @@
> > void _C_insert (const iterator &__it,
> > _IntType __n, _IntType __x, int) {
> > // see 23.1.1, p9 and DR 438
> > - _C_insert_n (__it, __n, __x);
> > + _C_insert_n (__it, __n, const_reference (__x));
>
> I suspect this is not correct. Suppose the type of __x is
> a 4 byte int and const_reference is an 8 byte long. The cast
> will make _C_insert_n() to read 4 bytes past the end of __x.
> A better fix might be to cast __x to value_type, as long as
> doing so doesn't violate [sequence.reqmts].
Yep. Good call. Will change to value_type() cast.
Brad.
>
> See also LWG DR 438 for some background:
> http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#438
>
> Martin
>
Re: [STDCXX-550] Please peer review this change.
Posted by Martin Sebor <se...@roguewave.com>.
Eric Lemings wrote:
>
> Would like to get another set of eyes on this before I submit.
>
> $ svn diff include/deque
> Index: include/deque
> ===================================================================
> --- include/deque (revision 662487)
> +++ include/deque (working copy)
> @@ -749,7 +749,7 @@
> void _C_insert (const iterator &__it,
> _IntType __n, _IntType __x, int) {
> // see 23.1.1, p9 and DR 438
> - _C_insert_n (__it, __n, __x);
> + _C_insert_n (__it, __n, const_reference (__x));
I suspect this is not correct. Suppose the type of __x is
a 4 byte int and const_reference is an 8 byte long. The cast
will make _C_insert_n() to read 4 bytes past the end of __x.
A better fix might be to cast __x to value_type, as long as
doing so doesn't violate [sequence.reqmts].
See also LWG DR 438 for some background:
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#438
Martin