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

Introduce CCCL clang-format #551

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

gevtushenko
Copy link
Collaborator

Description

closes #544

This PR introduces a CCCL-level clang-format file. There are a few minor differences compared to the original Thrust/CUB file.
We also temporarily specialize clang-format for libcu++ not to reorder includes.
Ideally, include order should not matter or at least all necessary places should be guarded with clang-format off/on. We prefer not waiting for all the fixes and get clang-format sooner. Once libcu++ include order issues are addressed, we can remove associated clang-format specialization.

Checklist

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

@gevtushenko gevtushenko requested review from a team as code owners October 11, 2023 20:12
@gevtushenko gevtushenko requested review from griwes, miscco and elstehle and removed request for a team October 11, 2023 20:12
@miscco
Copy link
Collaborator

miscco commented Oct 14, 2023

We should discuss this internally. I am personally not a fan of

BraceWrapping:
  AfterControlStatement: true
  BeforeCatch: true
  BeforeElse: true

It needlessly clutters the code as we have a lot of small conditionals in libcu++

    if (__other.__has_val_)
    {
      _LIBCUDACXX_CONSTRUCT_AT(this->__union_.__val_, __other.__union_.__val_);
    }
    else
    {
      _LIBCUDACXX_CONSTRUCT_AT(this->__union_.__unex_, __other.__union_.__unex_);
    }

vs

    if (__other.__has_val_) {
      _LIBCUDACXX_CONSTRUCT_AT(this->__union_.__val_, __other.__union_.__val_);
    } else {
      _LIBCUDACXX_CONSTRUCT_AT(this->__union_.__unex_, __other.__union_.__unex_);
    }

The former just introduces a lot of visual noise for libcu++. On the other hand @senior-zero has some valid points about larger conditionals in cub, so its not a hill I would die on

@gevtushenko
Copy link
Collaborator Author

@jrhemstad there's a consensus on merging this PR. One issue is that it requires clang-format 17, which we still don't have in our devcontainers. Current position is that the clang format file can be useful before devcontainers are updated because one can easily install clang-format-17 in our devcontainer and we do not force its usage for now. If you agree with this, please, merge the PR.

@miscco miscco merged commit 70395cd into NVIDIA:main Oct 19, 2023
511 checks passed
@jrhemstad jrhemstad mentioned this pull request Apr 5, 2024
2 tasks
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.

[FEA]: Create unified CCCL clang-format file
3 participants