HP aCC 3.63 compilation error in 21.string.append.stdcxx-438.cpp

[Martin Sebor <se...@roguewave.com>]

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

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


aCC -c     -I$(TOPDIR)/include -I$(BUILDDIR)/include 
-I$(TOPDIR)/tests/include  -AA  +O2   +w +W392,655,684,818,819,849 
$(TOPDIR)/tests/regress/21.string.append.stdcxx-438.cpp
Error 280: "$(TOPDIR)/include/string.cc", line 523 # Operator & expects 
lvalue operand.
                 _RWSTD_REINTERPRET_CAST (const_pointer, &*__first2);
                 ^^^^^^^^^^^^^^^^^^^^^^^
Error 280: "$(TOPDIR)/include/string.cc", line 525 # Operator & expects 
lvalue operand.
                 _RWSTD_REINTERPRET_CAST (const_pointer, &*__last2);
                 ^^^^^^^^^^^^^^^^^^^^^^^
Error 556: "$(TOPDIR)/tests/regress/21.string.append.stdcxx-438.cpp", 
line 85 # Unable to generate specialization 
"std::basic_string<char,std::char_traits<char>,std::allocator<char> > 
&std::__rw_replace<char,std::char_traits<char>,std::allocator<char>,char 
*,InputIterator>(std::basic_string<char,std::char_traits<char>,std::allocator<char> 
 > &,char *,char *,InputIterator,InputIterator)" due to errors during 
generation.
             str.append (first, last);
             ^^^^^^^^^^^^^^^^^^^^^^^^
Error 556: "$(TOPDIR)/tests/regress/21.string.append.stdcxx-438.cpp", 
line 85 # Unable to generate specialization 
"std::basic_string<char,std::char_traits<char>,std::allocator<char> > 
&std::__rw_replace<char,std::char_traits<char>,std::allocator<char>,char 
*,InputIterator>(std::basic_string<char,std::char_traits<char>,std::allocator<char> 
 > &,char *,char *,InputIterator,InputIterator)" due to errors during 
generation.
             str.append (first, last);
             ^^^^^^^^^^^^^^^^^^^^^^^^
gmake: *** [21.string.append.stdcxx-438.o] Error 2

Re: HP aCC 3.63 compilation error in 21.string.append.stdcxx-438.cpp

[Martin Sebor <se...@roguewave.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
>>>>
>>

RE: HP aCC 3.63 compilation error in 21.string.append.stdcxx-438.cpp

[Travis Vitek <Tr...@roguewave.com>]

 

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. If it is,
then we know it is safe to use the pointer to check for overlap.

>(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

  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?

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.

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


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

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

Re: HP aCC 3.63 compilation error in 21.string.append.stdcxx-438.cpp

[Martin Sebor <se...@roguewave.com>]

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. 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? (I'm still uncomfortable
with the cast and would like to understand why it's safe).

> 
> 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?

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

RE: HP aCC 3.63 compilation error in 21.string.append.stdcxx-438.cpp

[Travis Vitek <Tr...@roguewave.com>]

 

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

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

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
>