Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide adapter for std::reverse_iterator to replace boost::reverse_iterator for proxy reference iterators #2333

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

nvmkuruc
Copy link
Collaborator

@nvmkuruc nvmkuruc commented Mar 14, 2023

Description of Change(s)

Depends on #2205, #2328

#2205 replaces std::reverse_iterator in several but not all instances. The C++ standard states std::reverse_iterator::operator-> should return the equivalent of std::addressof(operator*()). For iterators that return proxy types as references (say PcpNodeRef), taking the address of a temporary is a problematic operation. The C++20 standard modifies this specification to std::prev(/*underlying iterator*/).operator->(). Some compilers are already using the newer specification.

This change introduces Tf_ProxyReferenceReverseIterator as an adapter for std::reverse_iterator that specializes operator-> to use the C++20 specification, deferring all other operations to std::reverse_iterator. When possible, this is explicitly done with using directives. However, in many cases, the return and argument types require wrapper methods and functions.

This change depends on #2328 due to a bug with the current implementation of PcpNodeIterator and std::prev.

Fixes Issue(s)

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@nvmkuruc nvmkuruc changed the title Provide an adapter for std::reverse_iterator to replace boost::reverse_iterator for proxy reference iterators Provide an adapter for std::reverse_iterator to replace boost::reverse_iterator for proxy reference iterators Mar 14, 2023
@nvmkuruc nvmkuruc changed the title Provide an adapter for std::reverse_iterator to replace boost::reverse_iterator for proxy reference iterators Provide adapter for std::reverse_iterator to replace boost::reverse_iterator for proxy reference iterators Mar 14, 2023
@tallytalwar
Copy link
Contributor

Filed as internal issue #USD-8122

}

// Operators and functions which can just use the underlying STL
// implementation
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these three functions are the only ones that work as is from std::reverse_iterator, I'm wondering if it would be better to use a private inherit of std::reverse_iterator and just explicitly implement the pass throughs to the parent for these functions also. The extra protection against converting to or returning as the underlying STL implementation seems beneficial.

@nvmkuruc nvmkuruc force-pushed the tfreverseiterator branch 2 times, most recently from cafcaab to 90dd255 Compare August 4, 2023 14:03
…erse_iterator` for proxy reference iterators
@pixar-oss pixar-oss merged commit eda8eeb into PixarAnimationStudios:dev Aug 16, 2023
@nvmkuruc nvmkuruc deleted the tfreverseiterator branch December 29, 2023 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants