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

Add clang tidy #129

Draft
wants to merge 12 commits into
base: dev
Choose a base branch
from
Draft

Add clang tidy #129

wants to merge 12 commits into from

Conversation

PointKernel
Copy link
Member

TBD

@PointKernel PointKernel marked this pull request as draft December 18, 2021 00:40
@jrhemstad jrhemstad added type: feature request New feature request Needs Review Awaiting reviews before merging and removed improvement labels May 19, 2022
Copy link
Collaborator

@sleeepyjack sleeepyjack left a comment

Choose a reason for hiding this comment

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

This PR satisfies my inner Monk. Very good!

@@ -29,6 +29,9 @@ include(rapids-find)
# * Enable the CMake CUDA language
rapids_cuda_init_architectures(CUCO)

# this is needed for clang-tidy runs
set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This also enables us to use other development tools that use the clangd language server. Awesome!

cuda_event_timer(benchmark::State &state, bool flush_l2_cache = false, cudaStream_t stream = 0)
cuda_event_timer(benchmark::State& state,
bool flush_l2_cache = false,
cudaStream_t stream = nullptr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default stream is commonly represented by (int)0 but I guess nullptr is fine, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

cudaStream_t is an alias of a pointer. clang-tidy will complain if we initialize it with 0.

@@ -83,7 +83,7 @@ __inline__ void test_custom_key_value_type(Map& map,
KeyIt key_begin,
size_t num_pairs)
{
constexpr cudaStream_t stream = 0;
constexpr cudaStream_t stream{nullptr};
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious about if we can use nullptr instead of 0.
https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#default-stream

Kernel launches and host <-> device memory copies that do not specify any stream parameter, or equivalently that set the stream parameter to zero, are issued to the default stream. They are therefore executed in order.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, 0 is the common way to initialize CUDA stream while according to the CUDA user guide, cudaStream_t is a type alias of CUstream_st * (see here) and clang-tidy will complain if it's initialized with 0. @karthikeyann @sleeepyjack For your reference, rmm is doing the same in rapidsai/rmm#857.

@PointKernel PointKernel added In Progress Currently a work in progress and removed Needs Review Awaiting reviews before merging labels Jul 1, 2022
@sleeepyjack
Copy link
Collaborator

What's the state of this PR? This could be used to automatically catch bugs such as #510.

@PointKernel
Copy link
Member Author

What's the state of this PR? This could be used to automatically catch bugs such as #510.

I vaguely recall that some upstream libraries couldn't be built with clang. Let me prioritize this work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Progress Currently a work in progress type: feature request New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants