Boost C++ Libraries: Ticket #8489: Unnecessary Definition of iterator_adaptor.hpp class Has Dangerous Return of Temporary Object

attachment set

Wed, 24 Apr 2013 00:42:20 GMT

attachment → iterator_adaptor.hpp.20130423.patch

A patch resolving the problem.

attachment set

Wed, 24 Apr 2013 00:42:43 GMT

attachment → iterator-adaptor.error-program.20130423.cc

Program illustrating the error.

summary changed

Wed, 24 Apr 2013 15:25:55 GMT

summary Unnecessary Definition of ??? Has Dangerous Return of Temporary Object → Unnecessary Definition of iterator_adaptor.hpp class Has Dangerous Return of Temporary Object

cc set

gromer@… — Wed, 24 Apr 2013 16:35:32 GMT

cc gromer@… added

oldham@… — Wed, 24 Apr 2013 19:47:59 GMT

The patch breaks iterator_facade.hpp::iterator_core_access::dereference() so it should not be used. However, the problem still exists for the test case.

oldham@… — Wed, 24 Apr 2013 23:45:17 GMT

It is not clear if the program showing the error is itself correct.

oldham@… — Thu, 25 Apr 2013 14:40:09 GMT

If my logic is correct, the return type for iterator_adaptor::dereference() is:

Reference if Reference != use_default,
iterator_reference<Base> if Reference == use_default and Value == use_default,
add_reference<Value> if Reference == use_default and Value != use_default.

I conjecture

the second case is not causing problems and
the third case is causing problems for the example program. For a slight variant of the program, I believe Value == const char and Reference == use_default. Thus, it tries to return a const char& which causes a local temporary to be created and then a reference returned.

One possible implementation solution is to specialize iterator_adaptor<> so dereference has a declaration with no definition for the third case. Another is to modify iterator_adaptor_base so problematic third cases do not occur.

It is not clear what to do.

james.hirschorn@… — Tue, 30 Apr 2013 18:06:11 GMT

I originally thought this was a bug too, but I do not believe it is.

First of all, the dereference member function of iterator_adaptor does not need to be valid (it is clearly invalid in your specific example). You can simply redefine dereference in your iterator class which inherits from iterator_adaptor, and the one from the base class will be ignored by the compiler.

Secondly, I supsect there may be a problem with the operator*() of your Base class. It is returning by value, but normally operator*() returns a reference.

Dave Abrahams — Mon, 10 Jun 2013 04:58:31 GMT

The problem here is that the underlying iterator of the iterator_adaptor is an istreambuf_iterator<char>, which quite legitimately (for an input iterator) returns a non-reference type from its operator*. This should be filed as a bug against Spirit.

To debug it, I applied this patch to iterator_adaptor.hpp, and ran the failing program through stlfilt:

iterator_adaptor.hpp

-              old
+              new
 #include <boost/mpl/and.hpp>
 #include <boost/mpl/not.hpp>
 #include <boost/mpl/or.hpp>
+#include <boost/mpl/assert.hpp>
 #include <boost/type_traits/is_same.hpp>
 #include <boost/type_traits/is_convertible.hpp>
+#include <boost/type_traits/is_reference.hpp>
 #ifdef BOOST_ITERATOR_REF_CONSTNESS_KILLS_WRITABILITY
 # include <boost/type_traits/remove_reference.hpp>
 …
       // base_reference(), above, to get direct access to m_iterator.
       //
       typename super_t::reference dereference() const
+        { return *m_iterator; }
+      {
+          BOOST_MPL_ASSERT((
+              mpl::or_<
+                  mpl::not_<is_reference<typename super_t::reference> >
+                , is_reference<
+#ifdef BOOST_NO_CXX11_DECLTYPE
+                    typename boost::iterator_reference<Base>::type
+#else
+                    decltype(*m_iterator)
+#endif
+                  >
+              >));
+          return *m_iterator;
+      }
       template <
       class OtherDerived, class OtherIterator, class V, class C, class R, class D

Dave Abrahams — Mon, 10 Jun 2013 05:37:29 GMT

I take it back; this isn't a Spirit bug… yet. The error is in the user program. As http://www.boost.org/doc/libs/1_53_0/libs/spirit/doc/html/spirit/support/multi_pass.html makes quite clear, Spirit works only with forward iterators. It's not clear at all, however, why I'm unable to solve the problem by wrapping the istreambuf_iterator with Spirit's multi_pass. See #3999