incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: [STDCXX-550] Please peer review this change.
Date Tue, 03 Jun 2008 16:38:03 GMT
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

Mime
View raw message