Replace boost::reverse_iterator with std::reverse_iterator for proxy-less iterators

[nvmkuruc]

Description of Change(s)

std::reverse_iterator can be used whenever the underlying iterator's operator* returns true references (and not proxy temporary values).

The C++ specification states that until C++20, std::addressof(operator*()) should be used as std::reverse_iterator's implementation of operator->. This does not work when the result of operator* is a temporary.

For the remaining cases, explicit reverse iterator implementations will be required compilers consistently support the C++20 specification (which is compatible with proxy reference types). This is addressed by #2333.

Fixes Issue(s)

• Prefer std::reverse_iterator instead of boost::reverse_iterator #2202

• I have verified that all unit tests pass with the proposed changes

• I have submitted a signed Contributor License Agreement

[sunyab]

Filed as internal issue #USD-7924

[sunyab]

Hey @nvmkuruc, it looks like this change fails to build on MacOS. I've attached a log file showing the failures from one of the Azure builds. Any thoughts?

log.zip

[sunyab]

Hm, this also seems to fail on my Linux builds with gcc 6.3.1:

In file included from /opt/rh/devtoolset-6/root/usr/include/c++/6.3.1/bits/stl_algobase.h:67:0,
                 from /opt/rh/devtoolset-6/root/usr/include/c++/6.3.1/bits/char_traits.h:39,
                 from /opt/rh/devtoolset-6/root/usr/include/c++/6.3.1/string:40,
                 from /scratch/jenkins/workspace/LINUX_GAUNTLET/pxr/base/tf/hash.h:35,
                 from /scratch/jenkins/workspace/LINUX_GAUNTLET/pxr/base/tf/refPtr.h:430,
                 from /scratch/jenkins/workspace/LINUX_GAUNTLET/pxr/base/tf/weakPtr.h:34,
                 from /scratch/jenkins/workspace/LINUX_GAUNTLET/pxr/base/tf/declarePtrs.h:33,
                 from /scratch/jenkins/workspace/LINUX_GAUNTLET/pxr/usd/usd/common.h:31,
                 from /scratch/jenkins/workspace/LINUX_GAUNTLET/pxr/usd/usd/stage.h:31,
                 from /scratch/jenkins/workspace/LINUX_GAUNTLET/pxr/usd/usd/stage.cpp:25:
/opt/rh/devtoolset-6/root/usr/include/c++/6.3.1/bits/stl_iterator.h: In instantiation of ‘std::reverse_iterator<_Iterator>::pointer std::reverse_iterator<_Iterator>::operator->() const [with _Iterator = pxrInternal_v0_23__pxrReserved__::PcpNodeIterator; std::reverse_iterator<_Iterator>::pointer = boost::iterators::detail::operator_arrow_dispatch<pxrInternal_v0_23__pxrReserved__::PcpNodeRef, pxrInternal_v0_23__pxrReserved__::PcpNodeRef*>::proxy]’:
/scratch/jenkins/workspace/LINUX_GAUNTLET/pxr/usd/usd/stage.cpp:6269:16:   recursively required from ‘pxrInternal_v0_23__pxrReserved__::TfIterator<T, Reverse>::Iterator& pxrInternal_v0_23__pxrReserved__::TfIterator<T, Reverse>::operator->() [with T = std::pair<pxrInternal_v0_23__pxrReserved__::PcpNodeIterator, pxrInternal_v0_23__pxrReserved__::PcpNodeIterator>; bool Reverse = true; pxrInternal_v0_23__pxrReserved__::TfIterator<T, Reverse>::Iterator = pxrInternal_v0_23__pxrReserved__::PcpNodeReverseIterator]’
/scratch/jenkins/workspace/LINUX_GAUNTLET/pxr/usd/usd/stage.cpp:6269:16:   required from here
/opt/rh/devtoolset-6/root/usr/include/c++/6.3.1/bits/stl_iterator.h:174:16: error: taking address of temporary [-fpermissive]
       { return &(operator*()); }
                ^~~~~~~~~~~~~~
/opt/rh/devtoolset-6/root/usr/include/c++/6.3.1/bits/stl_iterator.h:174:29: error: could not convert ‘& std::reverse_iterator<_Iterator>::operator*<pxrInternal_v0_23__pxrReserved__::PcpNodeIterator>()’ from ‘std::reverse_iterator<pxrInternal_v0_23__pxrReserved__::PcpNodeIterator>::reference* {aka pxrInternal_v0_23__pxrReserved__::PcpNodeRef*}’ to ‘std::reverse_iterator<pxrInternal_v0_23__pxrReserved__::PcpNodeIterator>::pointer {aka boost::iterators::detail::operator_arrow_dispatch<pxrInternal_v0_23__pxrReserved__::PcpNodeRef, pxrInternal_v0_23__pxrReserved__::PcpNodeRef*>::proxy}’
       { return &(operator*()); }

[nvmkuruc]

I suspect these might resolve themselves (or at least error more consistently) once we remove the iterator_facade underpinnings. Let me prioritize that and rebase this on top that to be sure.

[nvmkuruc]

Evaluating rebased on top of a PR which replaces the underlying iterator_facades with explicit iterators.

[nvmkuruc]

I'm reducing the scope of this PR to only address the trivial replacement of boost::reverse_iterator with std::reverse_iterator. Newer versions of GCC provide a version of reverse_iterator that's compatible with proxy reference types but older versions and other compilers do not. C++20 updates the specification to be compatible with proxy reference types.