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 usage of boost::compressed_pair with explicit empty base optimization #2259

Merged
merged 3 commits into from
Jul 3, 2023

Conversation

nvmkuruc
Copy link
Collaborator

@nvmkuruc nvmkuruc commented Feb 8, 2023

Description of Change(s)

  • Update TfDenseHash{Set,Map} to explicitly use private inheritance to take advantage of the empty base optimization instead of indirectly through boost::compressed_pair
  • Decouples initialization of vector storage, HashFn, and Equality{Key,Element} in constructors
  • Updates constructors to use std::make_unique instead of new operator (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-make_unique)
  • Decouples swapping of vector storage, HashFn, and Equality{Key,Element} in swap methods
  • Remove references to compressed_pair in test for TfDenseHashMap
  • Remove spurious include of utility.hpp in pxr/base/tf/denseHash{Set,Map}.h
  • Remove spurious include of boost/compressed_pair.hpp in pxr/usd/sdf/childrenView.h

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-7993

@@ -56,7 +54,13 @@ template <
class EqualKey = std::equal_to<Key>,
unsigned Threshold = 128
>
class TfDenseHashMap

Choose a reason for hiding this comment

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

Okay, we don't want to do this this way. The semantics of TfDenseHashMap now BEING a HashFn and an EqualKey is not desirable.
What I think you really want to do is reimplement the internal member class _VectorHashFnEqualFn using empty base optimization explicitly instead of the compressed_pair

@@ -55,7 +53,12 @@ template <
class EqualElement = std::equal_to<Element>,
unsigned Threshold = 128
>
class TfDenseHashSet
class TfDenseHashSet :

Choose a reason for hiding this comment

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

Same comment here; we don't want the DenseHashSet inheriting from these function types.

@pixar-oss pixar-oss merged commit 7c9140f into PixarAnimationStudios:dev Jul 3, 2023
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