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

Deduplicate global implementation #139

Merged
merged 3 commits into from
Feb 6, 2024
Merged

Deduplicate global implementation #139

merged 3 commits into from
Feb 6, 2024

Conversation

t4c1
Copy link
Contributor

@t4c1 t4c1 commented Feb 5, 2024

Deduplicate buffer and USMcodepaths in global implementation.

Checklist

Tick if relevant:

  • [N/A] New files have a copyright
  • [N/A] New headers have an include guards
  • API is documented with Doxygen
  • [N/A] New functionalities are tested
  • Tests pass locally
  • Files are clang-formatted

Copy link
Contributor

@hjabird hjabird left a comment

Choose a reason for hiding this comment

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

The inlining of the code makes this a challenging game of spot-the-difference. Can you draw our attention to any important changes? Thanks!

src/portfft/dispatcher/global_dispatcher.hpp Show resolved Hide resolved
@t4c1
Copy link
Contributor Author

t4c1 commented Feb 6, 2024

Inlining was done in a separate commit, so you can look just at deduplication if you want.

@t4c1
Copy link
Contributor Author

t4c1 commented Feb 6, 2024

Also, I would say there are no really important changes. Maybe you can give the new variables I created from function parameters a look and check if their names look good.

Copy link
Contributor

@hjabird hjabird left a comment

Choose a reason for hiding this comment

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

LGTM.

@t4c1 t4c1 merged commit d5fe215 into main Feb 6, 2024
1 check passed
@t4c1 t4c1 deleted the deduplicate_global branch February 6, 2024 11:33
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