Re: [boost] [range] iterator_range::operator[] broken
Neil, let's keep this discussion on-list. It's important. On 4/14/2010 12:21 AM, Neil Groves wrote:
On Wed, Apr 14, 2010 at 7:13 AM, Eric Niebler <eric@boostpro.com> wrote:
On 4/13/2010 8:03 PM, Steven Watanabe wrote:
AMDG
Eric Niebler wrote:
Neil, I know you patched this a mere 5 hours ago, but it's still broken. The attached patch *really* fixes the problem, I think.
Whoa. Why is this only returning a real reference for pointers?
Yeah, I dunno. It's this way on the release branch, too. It's very, very seriously broken. I think we need to revert range on the release branch to the 1.42 version, and fast.
I would much prefer to revert just iterator_range.hpp as the change to handle iterator proxies is orthogonal to the other changes. The change as applied attempts to leverage the same mechanism as is used in the iterator_facade to provide a real reference or a proxy where appropriate for the index operator.
It is a breaking interface change in a core library. What is the technical reason for it?
Neil?
I need to take another look at the code and perhaps the operator[] of iterator_range is broken, but I believe that this issue aside that the other changes are working well. It would be extremely disappointing to revert all of the adaptors, and algorithms just because of this issue. I'm more than happy to fix the problem as soon as I can.
I'm seeing massive breakages all over trunk and release, not all of which are related to the changed return type of iterator_range::operator[]. For instance, there seems to be some new ambiguity in the "detail" symbol. See for instance: http://beta.boost.org/development/tests/release/developer/output/RW_WinXP_VC... I can't be certain at this point if RangeEx is responsible, but it's where I would start looking. Could you have a look and report back? == Volunteers Needed === If anybody else is reading this: please look at the regressions on the release branch here: http://beta.boost.org/development/tests/release/developer/summary.html. Pick a broken library that interests you and figure out why it's busted. Report back. -- Eric Niebler BoostPro Computing http://www.boostpro.com
On Wed, Apr 14, 2010 at 8:42 AM, Eric Niebler <eric@boostpro.com> wrote:
Neil, let's keep this discussion on-list. It's important.
On 4/14/2010 12:21 AM, Neil Groves wrote:
It is a breaking interface change in a core library. What is the technical reason for it?
A trac issue was raised regarding iterator_range handling of iterators that return proxies. The change was motivated by attempting to resolve the trac issue. Since this specific solution is worse than the original problem I have altered the iterator_range code and reverter the operator[] handling to the old behaviour. The other change to the core functionality is being more permissive with respect to potentially singular iterators. This change I have kept in iterator_range.
Neil?
I need to take another look at the code and perhaps the operator[] of iterator_range is broken, but I believe that this issue aside that the other changes are working well. It would be extremely disappointing to revert all of the adaptors, and algorithms just because of this issue. I'm more than happy to fix the problem as soon as I can.
I'm seeing massive breakages all over trunk and release, not all of which are related to the changed return type of iterator_range::operator[]. For instance, there seems to be some new ambiguity in the "detail" symbol. See for instance:
http://beta.boost.org/development/tests/release/developer/output/RW_WinXP_VC...
I can't be certain at this point if RangeEx is responsible, but it's where I would start looking. Could you have a look and report back?
I have previously looked over the 'broken' regression tests and did not see a connection to the Boost.Range changes. I will look deeper into the outstanding problems over the other libraries. I can look at the issue tonight. I'm sure that even if this is an issue with Boost.Range that it is easily remedied. I had not spotted the connection between Boost.Range and the accumulators regression failure. Do you have other examples where the updates to Boost.Range are the cause of the problems? I suspect that any issues will be minor and easily resolved since the header file separation means that old code is largely unaffected by the additional features.
== Volunteers Needed ===
If anybody else is reading this: please look at the regressions on the release branch here: http://beta.boost.org/development/tests/release/developer/summary.html. Pick a broken library that interests you and figure out why it's busted. Report back.
If volunteers spot issues that are related to Boost.Range changes I am very keen to hear back so that I can improve the backward compatibility.
-- Eric Niebler BoostPro Computing http://www.boostpro.com
Regards, Neil Groves
I'm seeing massive breakages all over trunk and release, not all of which are related to the changed return type of iterator_range::operator[]. For instance, there seems to be some new ambiguity in the "detail" symbol. See for instance:
http://beta.boost.org/development/tests/release/developer/output/RW_WinXP_VC...
I can't be certain at this point if RangeEx is responsible, but it's where I would start looking. Could you have a look and report back?
This has relationship to changes in Boost.Range. The error: ..\boost/smart_ptr/detail/sp_counted_impl.hpp(81) : error C2872: 'detail' : ambiguous symbol could be 'boost::detail' or 'boost::numeric::operators::detail' ..\boost/smart_ptr/detail/shared_count.hpp(87) : see reference to class template instantiation 'boost::detail::sp_counted_impl_p<X>' being compiled with Is because of a potential ambiguity between ::boost::detail and the ::boost::numeric::operators::detail. This is can be fixed by prefixing the changing virtual void * get_deleter( detail::sp_typeinfo const& ) to virtual void * get_deleter( boost::detail::sp_typeinfo const& ) The probability of Boost.Range changes causing other ambiguity with the detail namespace is very low since I chose to use the range_detail namespace to avoid exactly this type of issue. However I accept that I have changed header dependencies and this can trigger this type of event. For this specific example the solution seems very simple. Would you like be to apply the change to sp_counted_impl.hpp? Regards, Neil Groves
On 4/14/2010 1:49 AM, Neil Groves wrote:
Eric Niebler wrote:
there seems to be some new ambiguity in the "detail" symbol. See for instance:
http://beta.boost.org/development/tests/release/developer/output/RW_WinXP_VC...
I can't be certain at this point if RangeEx is responsible, but it's where I would start looking. Could you have a look and report back?
This has relationship to changes in Boost.Range. The error:
---------^^ "no relationship"
The probability of Boost.Range changes causing other ambiguity with the detail namespace is very low since I chose to use the range_detail namespace to avoid exactly this type of issue. However I accept that I have changed header dependencies and this can trigger this type of event. For this specific example the solution seems very simple. Would you like be to apply the change to sp_counted_impl.hpp?
The joke is on me. It seems a change I made to accumulators is triggering this problem on some compilers. I'm investigating now. -- Eric Niebler BoostPro Computing http://www.boostpro.com
participants (2)
-
Eric Niebler -
Neil Groves