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

Remove use of the register keyword #2204

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Jan 26, 2023

Description of Change(s)

The register keyword is no longer allowed as of C++17. It's also now considered to be a fairly weak hint that doesn't do anything in most scenarios.
As a result, I've removed all uses of it in the lexer files, which allows USD to build as a C++17 project. The builds seem to work just fine without it, and some brief testing shows no difference in performance.

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement (Under the Apple CLA)

@dgovil dgovil changed the base branch from release to dev January 26, 2023 22:16
@spiffmon
Copy link
Member

Thanks, @dgovil - we're looking into upgrading our truly ancient version of flex to one that has the fix. If we don't manage to pull that off for our VFX2022 toolset, we'll definitely take this PR as a stop-gap.

@sunyab
Copy link
Contributor

sunyab commented Jan 27, 2023

Filed as internal issue #USD-7923

@stilllman
Copy link

We had to make the same fix for C++17 compatibility, I was about to push a PR when I saw this one. Note that the register keyword is used in two other files, pxr/base/arch/stackTrace.cpp and third_party/renderman-24/plugin/hdPrman/virtualStructConditionalGrammar.lex.cpp.

@dgovil
Copy link
Collaborator Author

dgovil commented Mar 14, 2023

Ah good call out. I'll remove it there too and update this PR just in case it's useful.

@spiffmon were there any updates on changing your flex version? Would it make sense to accept this PR in the interim, since they wouldn't conflict?

@dgovil
Copy link
Collaborator Author

dgovil commented Mar 15, 2023

@stilllman updated the PR with the files you suggested. A quick ripgrep search didn't show other uses, so thanks for catching those.

@pixar-oss pixar-oss merged commit 0d9f904 into PixarAnimationStudios:dev Apr 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.

5 participants