Mailing-List: contact dev-help@stdcxx.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@stdcxx.apache.org
Received-SPF: pass (athena.apache.org: domain of msebor@gmail.com designates
 209.85.132.244 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:date:from:organization:user-agent:mime-version:to
         :subject:references:in-reply-to:content-type
         :content-transfer-encoding:sender;
        b=DoR4TCCdIF4+JkL7AryteTIwkNgR7qXtiCZ59vjNRijFaVHHBLM1WeSvQYjRsRFrJz
         dO/7wcPiMV0be1Qu/fgbb9vKAyLUjKup1jul+00RT1dcMZ5NQcgjvRJG+pOH3HQfZ+pg
         IjApf48wAW9UHyaiIGQHBwoKEyaVKwnYK1dPc=
Message-ID: <48619B3D.8000805@roguewave.com>
Date: Tue, 24 Jun 2008 19:11:25 -0600
From: Martin Sebor <sebor@roguewave.com>
Organization: Rogue Wave Software, Inc.
User-Agent: Thunderbird 2.0.0.14 (X11/20080501)
MIME-Version: 1.0
To: dev@stdcxx.apache.org
Subject: Re: HP aCC 3.63 compilation error in 21.string.append.stdcxx-438.cpp
References: <48615B27.8070205@roguewave.com>
 <CFFDD219128FD94FB4F92B99F52D0A499AA8CC@exchmail01.Blue.Roguewave.Com>
 <48617603.4030003@roguewave.com>
 <CFFDD219128FD94FB4F92B99F52D0A499AA8D1@exchmail01.Blue.Roguewave.Com>
In-Reply-To: 
 <CFFDD219128FD94FB4F92B99F52D0A499AA8D1@exchmail01.Blue.Roguewave.Com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Sender: Martin Sebor <msebor@gmail.com>

Travis Vitek wrote:
>  
> 
> Martin Sebor wrote:
>> Travis Vitek wrote:
>>>> Martin Sebor wrote:
>>>>
>>>> The test fails to compile with HP aCC 3.63. The error messages
>>>> point to the patch for the issue:
>>>>
>>>>   http://svn.apache.org/viewvc?view=rev&revision=629550
>>> Actually, I think it was me that caused the problem, in the following
>>> change:
>>>
>>>     http://svn.apache.org/viewvc?view=rev&revision=669747
>>>
>>> The fix was for STDCXX-170 and friends, but break on every 
>>> compiler when the Iterator::operator*() returns a temporary
>>> (which is legal).
>> That's what I thought was your objection to Farid's patch.
> 
> Yes, it was. Unfortunately I forgot about this issue part way through
> making my patch.
> 
>> Am I
>> to understand that the code somehow detects whether operator*()
>> returns a rvalue or lvalue and the branch with the cast is only
>> supposed to be entered for lvalues?
> 
> Sort of. The code checks that the _InputIter is a pointer.

Oh. I missed this. I actually haven't reviewed your patch yet
so I've been mostly looking at Farid's change from March. Let
me reiterate what I mentioned in my comments on a similar but
unrelated change:
   http://www.nabble.com/forum/Permalink.jtp?root=13349768

I would just as soon delete rw/_typetraits from 4.2.x.

> If it is,
> then we know it is safe to use the pointer to check for overlap.

I see. But because we're using runtime dispatch instead of
compile-time one the code gets instantiated even for other
types besides pointers... and hence the aCC error (possibly
also due to a compiler bug).

> 
>> (I'm still uncomfortable
>> with the cast and would like to understand why it's safe).
> 
> It seems that the cast itself is legal because expr.static.cast says

I was just trying to understand what in the function guaranteed
that the code wouldn't be executed for non-pointer types. I got
thrown by the compiler error mentioning a user-defined iterator
type that clearly returned an rvalue.

> 
>   An expression e can be explicitly converted to a type T
>   using static_cast of the form static_cast<T>(e) if the
>   declaration "T t(e);" is well-formed, for some invented
>   temporary variable t.
> 
> The declaration
> 
>   const_reference t(*__first);
> 
> is legal if *__first is convertible to value_type, which is required, so
> everything seems okay here, right?

Right. But the same isn't okay for __last because it's not
dereferenceable. We'd need to compute the difference between
the two pointers and use it instead.

> 
> The problem with the original testcase was that the cast can end up
> giving us a pointer to a temporary if *__first doesn't return a
> reference, which can result in invalid results. The testcase I provided
> showed this.

Right.

> 
> My fix eliminated the cast (which caused breakage), but verifies that
> __first is actually a raw pointer type before trying to get a reference
> out of it.
> 
> So I think that we may be able to combine the two patches to come up
> with a usable fix.

I think we need to do the dispatch at compile time rather than at
runtime (despite the horrible __is_bidirectional_iterator kludge).
That way we'll avoid the cast and problems like the one we've just
run into with aCC.

 > If we avoid doing the cast until we know that __first
> is a pointer, then we can be sure that the cast is giving us reliable
> results. If __first is not a pointer, then all bets are off and we have
> to fall back to making a copy.

Sure.

> 
> 
>>> It looks like I'd need to do a special case when the iterator type is
>>> pointer. I don't see any way to legally check for no overlap without
>>> that, so the only option I can see then is to always make 
>> the copy and
>>> fix it with an overload selection trick (which would only be 
>> appropriate
>>> for 4.3.x).
>> I think it should be fine to optimize just the common case
>> (const_pointer) and leave the rest unoptimized (i.e., make
>> a copy). Or can you think of another common use that should
>> be optimized as well?
> 
> I think all cv-qualified pointer types could be optimized in this way. 

Yes. I was thinking of "common" non-pointer types such as
reverse_iterators (I'm not sure how common they are).

> 
>>> Travis 
>>>
>>>> Looking at the patch I don't see how the reinterpret_cast to
>>>> const_reference can possibly be correct, and I'm not sure we
>>>> satisfactorily resolved the same question the first time it
>>>> was raised back in March:
>>>>
>>>>   http://markmail.org/message/eijfmt3wxhg25bvs
>>>>
>>>> Farid?
>>>>
>>>> Thanks
>>>> Martin
>>>>
>>