Opened 9 years ago
Last modified 9 years ago
#8489 new Bugs
Unnecessary Definition of iterator_adaptor.hpp class Has Dangerous Return of Temporary Object
Reported by: | Owned by: | jeffrey.hellrung | |
---|---|---|---|
Milestone: | To Be Determined | Component: | iterator |
Version: | Boost 1.53.0 | Severity: | Problem |
Keywords: | Cc: | gromer@… |
Description
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.
Attachments (2)
Change History (10)
by , 9 years ago
Attachment: | iterator_adaptor.hpp.20130423.patch added |
---|
by , 9 years ago
Attachment: | iterator-adaptor.error-program.20130423.cc added |
---|
Program illustrating the error.
comment:1 by , 9 years ago
Summary: | Unnecessary Definition of ??? Has Dangerous Return of Temporary Object → Unnecessary Definition of iterator_adaptor.hpp class Has Dangerous Return of Temporary Object |
---|
comment:2 by , 9 years ago
Cc: | added |
---|
comment:3 by , 9 years ago
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.
comment:5 by , 9 years ago
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.
comment:6 by , 9 years ago
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.
comment:7 by , 9 years ago
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 18 18 #include <boost/mpl/and.hpp> 19 19 #include <boost/mpl/not.hpp> 20 20 #include <boost/mpl/or.hpp> 21 #include <boost/mpl/assert.hpp> 21 22 22 23 #include <boost/type_traits/is_same.hpp> 23 24 #include <boost/type_traits/is_convertible.hpp> 25 #include <boost/type_traits/is_reference.hpp> 24 26 25 27 #ifdef BOOST_ITERATOR_REF_CONSTNESS_KILLS_WRITABILITY 26 28 # include <boost/type_traits/remove_reference.hpp> … … 303 305 // base_reference(), above, to get direct access to m_iterator. 304 306 // 305 307 typename super_t::reference dereference() const 306 { return *m_iterator; } 308 { 309 BOOST_MPL_ASSERT(( 310 mpl::or_< 311 mpl::not_<is_reference<typename super_t::reference> > 312 , is_reference< 313 #ifdef BOOST_NO_CXX11_DECLTYPE 314 typename boost::iterator_reference<Base>::type 315 #else 316 decltype(*m_iterator) 317 #endif 318 > 319 >)); 320 return *m_iterator; 321 } 307 322 308 323 template < 309 324 class OtherDerived, class OtherIterator, class V, class C, class R, class D
comment:8 by , 9 years ago
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
A patch resolving the problem.