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.

Sign up for GitHub

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

Jump to bottom

Replace boost hash usage with TfHash in pxr/base/tf #2173

Merged
merged 1 commit into from
Feb 16, 2023
Merged

Replace boost hash usage with TfHash in pxr/base/tf #2173

merged 1 commit into from
Feb 16, 2023

Conversation

nvmkuruc
Copy link
Collaborator

@nvmkuruc nvmkuruc commented Jan 18, 2023 •
edited
Loading

Description of Change(s)

To remove the dependency of pxr/base/tf on boost's hashing functions

  • fileUtils replaces boost::hash_combine with TfHash::Combine
  • TfRefPtr and TfWeakPtrFacade now implement hash_value in terms of the existing TfHashAppend
  • Hash test coverages is added to TfRefPtr and TfWeakPtr tests
  • Until support for std::hash is enabled, an overload for std::type_index to TfHashAppend is now provided
  • Support is added for std::set and std::map to TfHash. This will facilitate changes changes in other downstream libraries.

Fixes Issue(s)

-#2172 (first of several forthcoming PRs)

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

@sunyab
Copy link
Contributor

sunyab commented Jan 18, 2023

Filed as internal issue #USD-7884

This was referenced Jan 18, 2023
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@nvmkuruc nvmkuruc force-pushed the tfhash branch 3 times, most recently from 442e846 to 3e9dd37 Compare January 27, 2023 22:59
spiffmon
spiffmon requested changes Feb 11, 2023
View reviewed changes
Copy link
Member

@spiffmon spiffmon left a comment

Choose a reason for hiding this comment

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

Hey @nvmkuruc , passing on a few notes from Alex, Ed, and Sunya, mostly identifying a few patterns for this and other PR's. With these addressed, the other PR's in this sequence should go much more quickly. Thank you!!

pxr/base/tf/hash.h Outdated Show resolved Hide resolved
pxr/base/tf/hash.h Outdated Show resolved Hide resolved
pxr/base/tf/hash.h Outdated Show resolved Hide resolved
pxr/base/tf/hash.h Outdated Show resolved Hide resolved
pxr/base/tf/hash.h Outdated Show resolved Hide resolved
pxr/base/tf/testenv/weakPtr.cpp Outdated Show resolved Hide resolved
pxr/base/tf/testenv/weakPtr.cpp Outdated Show resolved Hide resolved
@nvmkuruc nvmkuruc force-pushed the tfhash branch 2 times, most recently from 3a9b3ba to aad7f6e Compare February 13, 2023 17:42
spiffmon
spiffmon reviewed Feb 14, 2023
View reviewed changes
Copy link
Member

@spiffmon spiffmon left a comment

Choose a reason for hiding this comment

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

Just two little cleanliness things - thanks, @nvmkuruc !

pxr/base/tf/testenv/hash.cpp Outdated Show resolved Hide resolved
pxr/base/tf/weakPtrFacade.h Outdated Show resolved Hide resolved
@pixar-oss pixar-oss merged commit 161b055 into PixarAnimationStudios:dev Feb 16, 2023
@nvmkuruc nvmkuruc mentioned this pull request Feb 17, 2023
Merged
2 tasks
@nvmkuruc nvmkuruc deleted the tfhash branch March 2, 2023 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spiffmon spiffmon Awaiting requested review from spiffmon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nvmkuruc @sunyab @spiffmon @pixar-oss