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

Apply clang-format to cub #1602

Merged
merged 8 commits into from
Apr 10, 2024
Merged

Apply clang-format to cub #1602

merged 8 commits into from
Apr 10, 2024

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Apr 5, 2024

Description

This applies clang-format to CUB. With #1588 and the pre-commit.ci bot, autoformatting can be applied to any PR by commenting pre-commit.ci autofix.

All changes were automatically applied aside from the edits to .pre-commit-config.yaml.

See also:

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@bdice bdice requested review from a team as code owners April 5, 2024 22:12
@bdice
Copy link
Contributor Author

bdice commented Apr 5, 2024

This is a starting point for discussion -- I am open to any/all feedback on this PR. Some questions:

  • Is the .clang-format configuration exactly what we want, or should we tweak it first?
  • Are there any paths that we should exclude? e.g. vendored files that we want to keep the same as their upstream copy?
  • Do we want to break this down and apply it in chunks somehow?
  • How should we time the merge of this change to reduce conflicts with open PRs?

@alliepiper
Copy link
Contributor

Do we want to break this down and apply it in chunks somehow?

This might be a necessity, the changes view is crashing my browser 😄

Maybe 1 PR per project?

@wmaxey
Copy link
Member

wmaxey commented Apr 5, 2024

I'm under the impression we're going to want to exclude libcudacxx at least temporarily. We need to prevent clang-format from touching nv/target and probably figure out if we need a different format for the rest of the headers.

@jrhemstad
Copy link
Collaborator

I'm under the impression we're going to want to exclude libcudacxx at least temporarily. We need to prevent clang-format from touching nv/target and probably figure out if we need a different format for the rest of the headers.

We agreed in #551 to just have a single .clang-format for everything. I understand omitting nv/target, but everything else should be fair game.

@bdice
Copy link
Contributor Author

bdice commented Apr 5, 2024

Great, thanks for the feedback. I'll attempt to handle both comments above at the same time: I tweaked this and applied it only to CUB, to start. CUB has the smallest diff out of CUB, libcudacxx, and Thrust.

@bdice bdice changed the title Apply clang-format to cub, libcudacxx, thrust Apply clang-format to cub Apr 5, 2024
@wmaxey
Copy link
Member

wmaxey commented Apr 5, 2024

I'm under the impression we're going to want to exclude libcudacxx at least temporarily. We need to prevent clang-format from touching nv/target and probably figure out if we need a different format for the rest of the headers.

We agreed in #551 to just have a single .clang-format for everything. I understand omitting nv/target, but everything else should be fair game.

I'm happy with that. I looked at the changes and the surgery done isn't too impactful. <nv/target> was excluded already for some reason.

@bdice
Copy link
Contributor Author

bdice commented Apr 6, 2024

@jrhemstad @wmaxey @alliepiper I'm a little unsure of the conclusion to draw from the discussion above. Should I make a change and apply clang-format to the entire repo? Or break up the formatting into one PR per subproject as @alliepiper proposed?

@miscco
Copy link
Contributor

miscco commented Apr 6, 2024

I believe we should drop any additional .clang-format files.

<nv/target> can be dealt with a // clang-format off comment at the beginning of the file and // clang-format on at the end

It will be a trainwreck in our libcu++ test suite as we often have terribly long lines there, but I am absolutely in favor of just getting something in over the current state

@miscco
Copy link
Contributor

miscco commented Apr 6, 2024

I saw some issues with the formatting in libcu++ though.

It was not properly aligning consecutive alias definitions

@alliepiper
Copy link
Contributor

alliepiper commented Apr 6, 2024

I'm a little unsure of the conclusion to draw from the discussion above. Should I make a change and apply clang-format to the entire repo?

Making a separate PR for each subproject will simplify reviewing and let us discuss and deal with each one's issues separately, since some will probably be easier to land than others.

@alliepiper
Copy link
Contributor

While we're at it, we should probably add something like this to track these big formatting sweeps: https://github.com/NVIDIA/nvbench/blob/main/.git-blame-ignore-revs

@bdice
Copy link
Contributor Author

bdice commented Apr 7, 2024

.git-blame-ignore-revs is a good idea, but it will have to be done in follow-up PRs because the commit hashes will not be known in advance due to squash merging.

@jrhemstad jrhemstad requested review from miscco and gevtushenko April 8, 2024 20:31
@jrhemstad jrhemstad requested review from a team and gonidelis and removed request for a team April 8, 2024 20:31
@bdice bdice removed request for a team, robertmaynard, elstehle and gonidelis April 9, 2024 17:06
Copy link
Collaborator

@gevtushenko gevtushenko left a comment

Choose a reason for hiding this comment

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

@bdice thank you for this effort! Setting up an auto-formatting was long overdue for CUB. I had a few minor fixes that I pushed to your branch.

@bdice
Copy link
Contributor Author

bdice commented Apr 10, 2024

@gevtushenko Awesome, thanks! Please merge when you’re ready. @miscco volunteered to do the follow-ups for enabling clang-format on Thrust and libcudacxx.

@gevtushenko gevtushenko merged commit eefcca8 into NVIDIA:main Apr 10, 2024
586 checks passed
@miscco
Copy link
Contributor

miscco commented Apr 10, 2024

Thanks a lot for working on this @bdice its awesome 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants