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

[Jeffrey D. Oldham <oldham@…>]

Change definition of private iterator_adaptor<...>::dereference to a declaration, avoiding illegal return of local temporary object:

error: returning reference to local temporary object [-Werror,-Wreturn-stack-address]

{ return *m_iterator; }

Changing to a declaration is semantically acceptable because the preceding comment and a code search shows that the function was made private to ensure it is not used in this base class.

This was revealed using Clang++ 3.0.6. It is a showstopper for people required to use Clang with warnings turned in errors.

[Jeffrey D. Oldham <oldham@…>]

A patch resolving the problem.

[Jeffrey D. Oldham <oldham@…>]

Program illustrating the error.

[oldham@…]

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@…]

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

[oldham@…]

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@…]

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]

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

@@ -18 +18 @@
 #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>
@@ -303 +305 @@
       // 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]

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