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

Support string_view initialization and lookup in TfToken #3053

Closed

Conversation

nvmkuruc
Copy link
Collaborator

@nvmkuruc nvmkuruc commented Apr 24, 2024

Description of Change(s)

TfToken is designed to optimize storage and comparison of commonly used strings like property names, values, and other path components. It currently provides two constructors, one for NULL-terminated C-strings and the other for std::string.

There are cases where a token may need to be constructed from a part of a std::string. This often necessitates an intermediate heap-allocated copy when no allocation or copy is needed at all if the token already exists in the registry. The path parser currently optimizes this case by introducing a stack allocated char[32] and copying and NULL-terminating strings short enough to fit in the buffer. However, this has not been generalized outside of the path parser, is limited to short strings, and still requires a copy.

This change introduces a std::string_view constructor and associated utilities. It benefits the current implementation by unifying the C-string and std::string code paths, avoiding templating and specialization.

It also replaces Find(std::string const&) with the more general Find(std::string_view).

A note is left in the TfToken::_Rep class about using a set that supports transparent hashing. This would allow lookups to transparently compare std::string_view against a TfToken::_Rep without _Rep storing an extra std::string_view field.

Fixes Issue(s)

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

pxr/base/tf/token.h Outdated Show resolved Hide resolved
pxr/base/tf/token.h Outdated Show resolved Hide resolved
pxr/base/tf/token.h Outdated Show resolved Hide resolved
pxr/base/tf/token.h Outdated Show resolved Hide resolved
@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-9588

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nvmkuruc nvmkuruc force-pushed the tftokenstringview branch 2 times, most recently from 9993e62 to 324415e Compare April 25, 2024 20:48
@nvmkuruc nvmkuruc force-pushed the tftokenstringview branch from 324415e to 92b130b Compare May 20, 2024 19:44
@nvmkuruc nvmkuruc force-pushed the tftokenstringview branch from 92b130b to 6e43ed5 Compare June 10, 2024 18:01
@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Denise-Yang
Copy link

Closing this PR out because this change causes _Rep to increase in size, and when we upgrade to C++23, we will be able to address the issue that this PR brings up without needing to bloat _Rep.

@pixar-oss pixar-oss closed this Dec 16, 2024
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