Re: HP aCC 3.63 compilation error in 21.string.append.stdcxx-438.cpp
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(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
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(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
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
>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 >