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

Replace boost::iterator_range usage for ranges of UsdPrims #2280

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

nvmkuruc
Copy link
Collaborator

@nvmkuruc nvmkuruc commented Feb 15, 2023

Description of Change(s)

Depends on #2235

  • Implement UsdPrim{Sibling,Subtree}Range using the existing doxygen stubs
  • Relocate class declarations to after their corresponding iterator declarations
  • Make templated constructor and operators explicit
  • Add cbegin and cend methods
  • Remove unused references to boost::iterator_range in pxr/usd/usd/primData.h
  • Add test coverage for equality operators of UsdPrimSubtreeRange to testUsdPrimGetDescendants

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 force-pushed the usdrange branch 2 times, most recently from b9da8b7 to f58f549 Compare February 16, 2023 15:35
@tallytalwar
Copy link
Contributor

Filed as internal issue #USD-8018

UsdPrimSiblingRange(UsdPrimSiblingIterator begin,
UsdPrimSiblingIterator end) : _begin(begin),
_end(end) {}

Choose a reason for hiding this comment

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

I'm assuming these templated constructors and assignment operators are to perfectly match what boost::iterator_range provides?
I feel like these are overkill given that I would never expect anyone to construct a sibling range without calling the Get*Children functions that return them (and it would be honestly dangerous to do so since sibling iteration is a proxy). How do you feel about just leaving these out?

/// Iterator reference_type.
typedef iterator::reference reference;

UsdPrimSiblingRange() = default;

Choose a reason for hiding this comment

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

I'm curious about having to explicit default the implicit copy/move constructors/assignment. Is this necessary because the templated constructors below create a user-defined copy constuctor when expanded against UsdPrimSiblingRange?
If we get rid of the templated constructor/assignment, can we get rid of these declarations too?

return lhs.equal(rhs);
}

/// Equality comparison.

Choose a reason for hiding this comment

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

I'm also wondering if the template comparision operators are really needed (though they seem slightly less unneeded than the templated constructors.

typedef iterator::value_type value_type;
/// Iterator reference_type.
typedef iterator::reference reference;

Choose a reason for hiding this comment

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

Exact same comments for this range as the sibling range.

@pixar-oss pixar-oss merged commit 17ab8ff into PixarAnimationStudios:dev Aug 16, 2023
12 of 20 checks passed
@nvmkuruc nvmkuruc deleted the usdrange 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