many iterator types do not work with types that implement unary operator&

• Key: STDCXX-612
• Type: Bug
• Status: Open
• Priority: Major
• Assignee: Farid Zaripov
• Reporter: Travis Vitek
• Component/s: 24. Iterators
• Affects Version/s: 4.2.0
• Fix Version/s: 4.3.0

Description

Code that uses the macro _RWSTD_OPERATOR_ARROW will be affected by this issue. Code that has '&*' is also very likely to be affected.

#include <deque>
#include <iterator>
#include <list>
#include <set>
#include <vector>

struct S
{
void operator& () const {};
};

int main ()
{
// this is just a compile test, it is not intended to run

std::reverse_iterator<S*>().operator->();
std::set<S>::iterator().operator->();
std::deque<S>::iterator().operator->();
std::list<S>::iterator().operator->();

return 0;
}

Comments

Rev 590116 (http://svn.apache.org/viewvc?view=rev&revision=590116) doesn't look like it goes with this issue.

[Farid Zaripov]

The patch is attached

[Farid Zaripov]

ChangeLog:

• include/rw/_defs.h: #define _RWSTD_ADDRESS_OF - new macro to
  obtain the memory address of the object.
  (_RWSTD_OPERATOR_ARROW): Add "type" parameter. Use
  _RWSTD_ADDRESS_OF() macro.
• include/deque (_RWSTD_OPERATOR_ARROW): Split signature to two
  parameters: type and rest.
• include/deque_spec.h (_RWSTD_OPERATOR_ARROW): Ditto.
• include/list (_RWSTD_OPERATOR_ARROW): Ditto.
• include/list_spec.h (_RWSTD_OPERATOR_ARROW): Ditto.
• include/rw/_iterator.h (_RWSTD_OPERATOR_ARROW): Ditto.
• include/rw/_iterbase.h (_RWSTD_OPERATOR_ARROW): Ditto.
• include/rw/_streamiter.h (_RWSTD_OPERATOR_ARROW): Ditto.
• include/rw/_tree.h (_RWSTD_OPERATOR_ARROW): Ditto.
• tests/include/alg_test.h (_RWSTD_OPERATOR_ARROW): Ditto.
• include/rw/specialized.h (_rw_address_of): New template function
  to get the address of the variable.
  (uninitialized_copy): use __rw_address_of() instead of operator&.
  (uninitialized_fill): Ditto.
• include/tr1/_smartptr.h (operator->): return _C_ptr instead of using
  operator*().

[Travis Vitek]

The change to tr1/_smartptr.h removes an assertion that _C_ptr is valid. I don't know if the assert was necessary in the first place, but it probably shouldn't be removed without reason.

Also, I think the _RWSTD_ADDRESS_OF() would be easy to accidentally misuse.

_RWSTD_ADDRESS_OF (int, i); // cast address of i to an int

Wouldn't it make more sense for __rw_address_of to do the heavy lifting, and to write the macro to call through? That would require the free function be moved to rw/_defs.h which might not be ideal. It has the advantage that would remove requirement for the first macro parameter, which would cleanup the code just a little bit. It might also be a good idea to add an overload of __rw_address_of for const references.

[Travis Vitek]

Also, some places in the standard have `Returns' and `Effects' clauses that say that the operator->() is supposed to return some specific thing. For example, 24.4.1.3.4 [revers.iter.opref] has an effects clause that claims `pointer operator->() const' returns &(operator*()). Of course, with that said, there is nothing that says how container iterators should behave when this situation comes up for their forward iterators. It seems to indicate that the containers should use reverse_iterator for their reverse iterator implementation, which, as mentioned above, is essentially required to not work with types that implement operator& to do something abnormal.

If we make some of these changes it will make our implementation non-compliant, which may be a bad thing. This is one of those issues where it might need to be brought up with the committee for discussion before we have a complete and satisfying solution. Of course the standards committee might have already covered this issue at one point, but if they have, they haven't documented it very well.

[Martin Sebor]

We need to be careful here. We can't change [revers.iter.opref] or any other function that deliberately relies on the return type of operator&() being [convertible to] pointer.

As for where things should be defined: _defs.h is for macro definitions only. There shouldn't be any other definitions (types, templates, or functions). As an aside, we should be using C++ casts in favor of the C-style ones.

Here's how I'd like to propose we proceed:

1) First, to simplify things, eliminate the _RWSTD_ARROW() macro. It's a workaround for a compiler limitation that none of our compilers suffers from anymore (check the generated config headers) and so this is a forward compatible change.

2) For standard container iterators define operator->() in terms of the pointer data member as opposed to in terms of operator&() and avoid having to deal with any conversion issues.

3) Leave reverse_iterator::operator->() unchanged (i.e., return &operator*()).

4) Change istream_iterator::operator->() to simply return &_C_val.

5) Change __rw_debug_iter::operator->() to simply return _C_iter.

6) Discuss how to deal with the uninitialized_xxx() algorithms.

[Martin Sebor]

Travis, let us know if this approach sounds reasonable to you or if you see a problem.

[Travis Vitek]

I'm a little on the fence as to whether item 1 above really is forward compatible or not. There may be some compiler that we aren't testing that requires it, and we would be breaking compatibility there.

I don't really have a problem with letting this issue sit until 4.3 or later. I'm also open to never fixing the issue if someone can provide a reasonable argument for why you would want it to not work. Of course that may require someone to explain why the standards gurus decided to require this interesting behavior in some places. That said, if we are intent on doing something with this issue now, then I think the above is acceptable.

[Martin Sebor]

I also have no problem deferring this issue for 4.3. It's very much a corner case. Let's see what Farid thinks about it.

As for compatibility, the [still informal] binary compatibility policy is silent on this point but I don't think we need to be concerned with hypothetical compilers when it comes to maintaining compatibility. IMO, we should only be constrained by compilers we do test and support.

[Farid Zaripov]

Deferred to 4.3 release.

[Martin Sebor]

Assigned to Travis.

[Martin Sebor]

Err, reassigned to Farid, the author of the attached patch.