-
Notifications
You must be signed in to change notification settings - Fork 89
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
base: dev
Are you sure you want to change the base?
Add clang tidy #129
Conversation
There was a problem hiding this 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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
TBD