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

Add TfDelegatedCountPtr to support custom bookkeeping #2891

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

nvmkuruc
Copy link
Collaborator

@nvmkuruc nvmkuruc commented Dec 28, 2023

Description of Change(s)

OpenUSD currently uses boost::intrusive_ptr to support customized bookkeeping of types. Specialization is achieved through ADL (intrusive_ptr_add_ref and intrusive_ptr_release).

This introduces TfDelegatedCountPtr as an alternative to support customized bookkeeping via ADL overloads TfDelegatedCountIncrement and TfDelegatedCountDecrement.

The TfDelegatedCountPtr avoids implicit conversion of raw pointers and requires explicit tag dispatch to specify whether the pointer should be retained or not.

This also provides a TfMakeDelegatedCountPtr creator function to avoid direct allocation.

Fixes Issue(s)

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

@tallytalwar
Copy link
Contributor

Filed as internal issue #USD-9122

@nvmkuruc nvmkuruc force-pushed the tfintrusiveptr branch 2 times, most recently from 755d12d to 8552493 Compare January 22, 2024 21:28
@nvmkuruc nvmkuruc changed the title Add Tf_TrackingPtr to support custom bookkeeping Add TfDelegatedCountPtr to support custom bookkeeping Jan 22, 2024
@nvmkuruc nvmkuruc force-pushed the tfintrusiveptr branch 2 times, most recently from fbe72b6 to 5715c18 Compare January 23, 2024 14:27
}

/// Dereference the underlying pointer
ReferenceType operator*() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

noexcept?

/// construction, assignment, and destruction operations.
///
/// `TfDelegatedCountIncrement` and `TfDelegatedCountDecrement` are expected to
/// be `noexcept`. The increment and decrement methods can assume pointers are
Copy link
Contributor

Choose a reason for hiding this comment

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

Requiring these to be noexcept seems potentially too restrictive to me... What's the motivation for this requirement? Could we just transparently propagate the noexcept-ness of these so that it's a "pay for what you use" type of thing?

/// construction, assignment, and destruction operations.
///
/// As it may be called in a destructor, `TfDelegatedCountDecrement` is expected
/// to be `noexcept`. The increment and decrement methods can assume pointers
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this comment now, no?


using IncrementIsNoExcept =
std::integral_constant<
bool, noexcept(TfDelegatedCountIncrement(RawPtrType{}))>;
Copy link
Contributor

Choose a reason for hiding this comment

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

could you use std::declval<RawPtrType>() for the value here?

}

/// Dereference the underlying pointer
ReferenceType operator*() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be noexcept I think.

///
/// `ptr` will be reset to its default state (ie. `nullptr`).
TfDelegatedCountPtr& operator=(TfDelegatedCountPtr&& ptr)
noexcept(DecrementIsNoExcept()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ok if I had a foo that was a TfDelegatedCountPtr<Foo> and I did:

foo = std::move(foo);

That would blow up, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a test case for this specifically. It doesn't blow up. foo is left in a valid state. See TestMovingSelf().


private:
void _IncrementIfValid() noexcept(IncrementIsNoExcept()) {
if (get()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not to just do if (_pointer) here? we use it directly in the body immediately after -- it seems clearer to me that way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A bunch of prior usages of _pointer had to be converted to get() to support operations on convertible types without friend-ship. But there's no reason this usage had to be converted.

@nvmkuruc nvmkuruc force-pushed the tfintrusiveptr branch 3 times, most recently from 9bcaf2c to e64b2a5 Compare February 11, 2024 15:58
@pixar-oss pixar-oss merged commit 5d48f55 into PixarAnimationStudios:dev Mar 1, 2024
3 of 5 checks passed
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.

4 participants