Apply clang-tidy autofixes #15894

vyasr · 2024-05-30T23:48:51Z

Description

This changeset is large, but it's not very substantial. It's all the automated fixes produced by clang-tidy using our script. The bulk of the changes are either adding [[nodiscard]] to many functions or changing const ref args to pass by value and then move in cases where the parameter is only used to set a value. There are also some places where clang-tidy preferred either more or less namespacing of objects depending on the current namespace. The goal is to enable clang-tidy in CI, which we made progress towards in #9860 but stalled in #10064. This PR contains the first set of changes that will required for such a check to pass.

I've marked this PR as breaking because some of the functions now marked as [[nodiscard]] are public APIs, so if consumers were ignoring the return values they will now see warnings, and if they are compiling with warnings as errors then the builds will break.

Contributes to #584

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

bdice

A few initial comments -- I got through 55 / 235 files and decided to share my comments before going further, since some apply throughout the PR.

cpp/include/cudf/detail/aggregation/aggregation.hpp

cpp/include/cudf/detail/structs/utilities.hpp

cpp/include/cudf/interop.hpp

cpp/include/cudf/io/types.hpp

cpp/include/cudf/join.hpp

cpp/include/cudf/utilities/span.hpp

cpp/src/io/parquet/predicate_pushdown.cpp

bdice · 2024-05-31T12:49:05Z

cpp/src/io/parquet/reader_impl_helpers.cpp

-                         find_schema_child(schema_elem, col_name_info->children[idx].name),
-                         output_col.children,
-                         has_list_parent || col_type == type_id::LIST);
+        for (const auto& idx : col_name_info->children) {


Lots of west-const in this PR... can we autoformat to east const to match the rest of the library? (We don't have this option enabled in clang-format because it's supposedly bug-prone, but I don't think we observed any actual bugs when we last ran a manual formatting pass.)

Hmm yeah at minimum I can do a manual pass on this PR and fix anything that goes wrong.

Manual pass worked fine. The warning is still present in the clang-format docs, though, so I've left the option out of our configuration.

I dug up the original reviews for the PR where this feature and that warning were added. https://reviews.llvm.org/D69764#inline-1055542

Basically the breaking cases are things like:

#define INTPTR int * const INTPTR a;

where a macro would mess up the semantics of const and * by swapping the order.

I'd be okay with accepting that edge case and enabling this setting across the code base.

Let's discuss this more in a follow-up. The rule applies to all qualifiers, not just const, so if we turn it on we'll also either need to accept "east volatile" or we will need to see if there are additional settings we can tweak to avoid that. Personally I would be fine with moving volatile as well, but I'd rather not do that in this PR since I'm sure that will be debated.

east const formatting does not work 100% with any tool. Each tool misses some. Each tool takes different approach to convert to east const. We can run a few tools to minimize it and some manual tweaks. I used westerly tool, and clang-tidy.

vyasr · 2024-05-31T17:54:11Z

I'm not sure what the right thing to do is for generated and vendored files. There are two in particular that are a bit problematic, cxxopts.hpp and Schema_generated.h. The problem is that the way clang-tidy works is recursive, i.e. you point it to a source file and it checks all headers included by that file. As a result, it's not sufficient to use something like pre-commit filtering of files beforehand to tell it what files to ignore. The way to specify what files to include is using the -header-filter argument to clang-tidy itself. However, this argument does not support lookahead/lookbehind in regexes, which is the only way I can imagine filtering out a specific set of files right now. The as-yet-unreleased version 19 includes a new feature to exclude headers, though. Our options are to either go ahead and apply changes to these files, or to wait until clang-tidy 19 is released and update to using that and assume that the new option will allow us to do the necessary filtering.

I should note that while we do currently have pre-commit filtering out the cxxopts.hpp file, the Schema_generated.h file has not been filtered, so we are already applying clang-format to it. That being the case, I would be fine with just removing the filters and applying clang-tidy and clang-format uniformly to both files, but I wanted to see what others thought first.

davidwendt · 2024-05-31T18:05:40Z

I should note that while we do currently have pre-commit filtering out the cxxopts.hpp file, the Schema_generated.h file has not been filtered, so we are already applying clang-format to it. That being the case, I would be fine with just removing the filters and applying clang-tidy and clang-format uniformly to both files, but I wanted to see what others thought first.

I'd rather add the Schema_generated.h to the pre-commit filter if that is possible.
I'm ok with updating the files manually using this PR and we can revisit again once version 19 is out.

I'm more concerned that we have some cpp-tests failing with these changes.

vyasr · 2024-05-31T18:33:33Z

@davidwendt apologies, I had fixed the failing test but just realized I was pushing to the wrong branch. It was just an issue of a missed NOLINT; clang-tidy doesn't understand all of the extra braces that we sometimes need in tests in order to make things like nested list columns work.

As I mentioned, we can't use a pre-commit filter:

The problem is that the way clang-tidy works is recursive, i.e. you point it to a source file and it checks all headers included by that file. As a result, it's not sufficient to use something like pre-commit filtering of files beforehand to tell it what files to ignore.

It's not really about pre-commit, it's about clang-tidy itself. If you run clang-tidy on any source file, it will also detect errors in any of the headers that file includes.

davidwendt · 2024-05-31T18:42:17Z

Ok, this part sounded like we have a mistake. I'm proposing we fix this:

I should note that while we do currently have pre-commit filtering out the cxxopts.hpp file, the Schema_generated.h file has not been filtered, so we are already applying clang-format to it.

And this part sounded like we can solve the clang-tidy issue with version 19:

The as-yet-unreleased version 19 includes a new feature to exclude headers, though.

I'm proposing we merge this PR (once it passes) and wait for 19.

vyasr · 2024-05-31T18:51:28Z

Got it. Yes, I can fix the exclusion for clang-format for Schema_generated.h

As far as waiting, I'm observing a biannual release cadence in March and September. I would (admittedly selfishly) like to get this work done before then personally, but I understand the desire to not have that behavior, so I'll defer to you on that if you think it's worth avoiding the modifications to the generated file.

cpp/src/io/parquet/predicate_pushdown.cpp

cpp/include/cudf/detail/structs/utilities.hpp

bdice

I have read enough of this that I think it can be merged. I didn't read the changes to cpp/tests but I read everything else.

Aside from the other comments I just left, I would like one more spot-check for things like indices -> indice in the for-loop simplifications. I'm not sure how to check for those. Maybe grep the diff for for (.*:.* or something like that.

…_tidy_fixes

vyasr · 2024-06-03T17:56:58Z

I did the spot-check and didn't find anything. Thanks for being so diligent!

I'll do a follow-up PR where I fix the clang-tidy errors that don't get autofixed. I figured this changeset was large enough as is.

cpp/include/cudf/detail/structs/utilities.hpp

karthikeyann · 2024-06-04T05:29:35Z

cpp/include/cudf/scalar/scalar.hpp

-  chrono_scalar()  = delete;
-  ~chrono_scalar() = default;
+  chrono_scalar()           = delete;
+  ~chrono_scalar() override = default;


Question: This is interesting. Why is override required here?

It is coming from this rule. In this particular case (a virtual, no-op destructor) the arguments on that page don't really apply. However, it does help in one way: it ensures a compiler error if the parent class's destructor is not declared virtual (which can result in UB when you have inheritance).

cpp/include/cudf/scalar/scalar_factories.hpp

karthikeyann · 2024-06-04T05:32:53Z

cpp/include/cudf/utilities/error.hpp

@@ -237,7 +237,7 @@ inline void throw_cuda_error(cudaError_t error, char const* file, unsigned int l
  // Calls cudaGetLastError to clear the error status. It is nearly certain that a fatal error
  // occurred if it still returns the same error after a cleanup.
  cudaGetLastError();
-  auto const last = cudaFree(0);
+  auto const last = cudaFree(nullptr);


Could this be detected using compiler warnings?

IIRC this can be detected with host compilers but not nvcc. I'd have to double-check that, though.

OK, yes, this can be enforced with -Wzero-as-null-pointer-constant. However, that leads to warnings from cuda_runtime_api.h. I'm not sure if there is a good way to avoid that.

cpp/include/cudf/scalar/scalar_factories.hpp

cpp/tests/copying/gather_str_tests.cpp

PointKernel · 2024-06-04T20:56:11Z

Question: do we enforce this linting in CI?

…_tidy_fixes

vyasr · 2024-06-05T01:29:04Z

Question: do we enforce this linting in CI?

Not yet. The plan is to get things fixed first, then add a CI check. However, that may have to wait a few months, see #15894 (comment).

srinivasyadav18

Changes looks good to me!

ttnghia

Java approval.

…_tidy_fixes

vyasr · 2024-06-11T22:16:21Z

/merge

karthikeyann · 2024-06-11T22:59:08Z

cpp/tests/copying/concatenate_tests.cpp

-  cudf::test::fixed_width_column_wrapper<int32_t> col1_1{{5, 4, 3, 5, 8, 5, 6},
-                                                         {0, 1, 1, 1, 1, 1, 1}};
+  cudf::test::fixed_width_column_wrapper<int32_t> col1_1{
+    {5, 4, 3, 5, 8, 5, 6}, {false, true, true, true, true, true, true}};


nit: my personal preference is 0, 1 style for validity mask, it is easy to read quickly, aligns well with data in above line.

karthikeyann · 2024-06-11T22:59:55Z

cpp/tests/copying/gather_str_tests.cpp

@@ -124,8 +125,8 @@ TEST_F(GatherTestStr, GatherDontCheckOutOfBounds)
                                      rmm::mr::get_current_device_resource());

  std::vector<char const*> h_expected;
-  for (auto itr = h_map.begin(); itr != h_map.end(); ++itr) {
-    h_expected.push_back(h_strings[*itr]);
+  for (int itr : h_map) {


why not auto here?

karthikeyann · 2024-06-11T23:04:48Z

cpp/tests/copying/slice_tests.cpp

+    {true,
+     true,
+     true,
+     true,
+     true,
+     true,
+     true,
+     true,
+     true,
+     true,
+     true,
+     true,
+     true,
+     true,
+     true,
+     true,
+     false,
+     false,
+     false}};


nit: same comment about validity mask. readability is better, if the each data aligns with its validity too.

karthikeyann · 2024-06-11T23:08:39Z

cpp/src/io/parquet/ipc/Schema_generated.h

@@ -139,13 +139,13 @@ inline const MetadataVersion (&EnumValuesMetadataVersion())[5]
  return values;
 }

-inline const char* const* EnumNamesMetadataVersion()
+inline char const* const* EnumNamesMetadataVersion()


This file has this comment on the top.
// automatically generated by the FlatBuffers compiler, do not modify
if it is possible to enforce east const with FlatBuffers compiler, please add that as comment in top of the file too.

…_tidy_fixes

The clang-tidy changes in rapidsai#15894 introduce a new exclude regex list to the pre-commit clang-format hook. However, it was a single character too long, ending with a |. Consequently, the exclude regex matched the empty string, and hence excluded every C++ file. Fix this, and apply formatting changes to the files that were modified in the interim and were not clang-format compatible.

The clang-tidy changes in #15894 introduce a new exclude regex list to the pre-commit clang-format hook. However, it was a single character too long, ending with a |. Consequently, the exclude regex matched the empty string, and hence excluded every C++ file. Fix this, and apply formatting changes to the files that were modified in the interim and were not clang-format compatible. Authors: - Lawrence Mitchell (https://github.com/wence-) Approvers: - David Wendt (https://github.com/davidwendt) - Kyle Edwards (https://github.com/KyleFromNVIDIA) - Bradley Dice (https://github.com/bdice) URL: #16030

KyleFromNVIDIA · 2024-08-27T18:06:04Z

#15894 (comment)

which is the only way I can imagine filtering out a specific set of files right now.

CMake handles this by putting vendored files in their own directory and then adding a .clang-tidy file to that directory. For example: https://gitlab.kitware.com/cmake/cmake/-/blob/72607d3402b91c3a9f4579c86175e4ca1ce6aa4e/Utilities/.clang-tidy

vyasr · 2024-08-27T19:10:54Z

That could definitely work too, although we'd potentially need a separate such directory for tests and cudf source. At this point I'm more inclined to wait for clang 19 since it's around the corner, but if for some reason the tooling that provides isn't sufficient I'd be happy to see the reorg. Alternatively, if you're motivated to get this done sooner feel free to try your approach. I agree that it ought to work and it should be pretty easy.

vyasr added 3 commits May 30, 2024 23:43

First pass of clang-tidy autofixes

b185302

More unambiguous fixes

8b59117

More unambiguous fixes

77bfaab

vyasr added libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function breaking Breaking change labels May 30, 2024

vyasr self-assigned this May 30, 2024

vyasr requested a review from a team as a code owner May 30, 2024 23:48

vyasr requested review from karthikeyann and srinivasyadav18 May 30, 2024 23:48

bdice reviewed May 31, 2024

View reviewed changes

vyasr added 3 commits May 31, 2024 18:22

Apply east const formatting everywhere

290daa2

Address PR feedback

ac8898a

Fix failing test

c950f24

vyasr requested a review from a team as a code owner May 31, 2024 18:22

github-actions bot added the Java Affects Java cuDF API. label May 31, 2024

Exclude generated and vendored files from linters

db45580

vyasr requested a review from a team as a code owner May 31, 2024 19:05

vyasr requested a review from KyleFromNVIDIA May 31, 2024 19:05

bdice reviewed May 31, 2024

View reviewed changes

cpp/src/io/parquet/predicate_pushdown.cpp Outdated Show resolved Hide resolved

cpp/src/io/parquet/predicate_pushdown.cpp Outdated Show resolved Hide resolved

cpp/include/cudf/detail/structs/utilities.hpp Show resolved Hide resolved

bdice approved these changes May 31, 2024

View reviewed changes

vyasr added 2 commits June 3, 2024 17:55

Fix name

3eb5508

Merge remote-tracking branch 'upstream/branch-24.08' into chore/clang…

f8e274f

…_tidy_fixes

karthikeyann reviewed Jun 4, 2024

View reviewed changes

srinivasyadav18 reviewed Jun 4, 2024

View reviewed changes

cpp/include/cudf/scalar/scalar_factories.hpp Outdated Show resolved Hide resolved

cpp/tests/copying/gather_str_tests.cpp Outdated Show resolved Hide resolved

vyasr added 5 commits June 5, 2024 01:03

Fix a pass-by-value

f5ccc94

Revert namespace removals

82b73a0

Minor fixes

9ec9cfe

Merge remote-tracking branch 'upstream/branch-24.08' into chore/clang…

e702b3e

…_tidy_fixes

Remove unnecessary ref

bbdf899

Revert changes to generated message file

e83aa4d

vyasr requested review from karthikeyann and srinivasyadav18 June 5, 2024 16:42

srinivasyadav18 reviewed Jun 5, 2024

View reviewed changes

ttnghia approved these changes Jun 5, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/branch-24.08' into chore/clang…

af3c475

…_tidy_fixes

karthikeyann reviewed Jun 11, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/branch-24.08' into chore/clang…

77ce6e3

…_tidy_fixes

rapids-bot bot merged commit 2b10299 into rapidsai:branch-24.08 Jun 12, 2024
74 checks passed

wence- mentioned this pull request Jun 14, 2024

Fix exclude regex in pre-commit clang-format hook #16030

Merged

3 tasks

vyasr deleted the chore/clang_tidy_fixes branch August 27, 2024 19:09

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply clang-tidy autofixes #15894

Apply clang-tidy autofixes #15894

vyasr commented May 30, 2024

bdice left a comment

bdice May 31, 2024

vyasr May 31, 2024

vyasr May 31, 2024

bdice May 31, 2024

vyasr Jun 3, 2024 •

edited

Loading

karthikeyann Jun 11, 2024

vyasr commented May 31, 2024

davidwendt commented May 31, 2024

vyasr commented May 31, 2024

davidwendt commented May 31, 2024

vyasr commented May 31, 2024

bdice left a comment •

edited

Loading

vyasr commented Jun 3, 2024

karthikeyann Jun 4, 2024

vyasr Jun 5, 2024 •

edited

Loading

karthikeyann Jun 4, 2024

vyasr Jun 5, 2024

vyasr Jun 5, 2024

PointKernel commented Jun 4, 2024

vyasr commented Jun 5, 2024

srinivasyadav18 left a comment

ttnghia left a comment

vyasr commented Jun 11, 2024

karthikeyann Jun 11, 2024

karthikeyann Jun 11, 2024

karthikeyann Jun 11, 2024

karthikeyann Jun 11, 2024

KyleFromNVIDIA commented Aug 27, 2024

vyasr commented Aug 27, 2024

Apply clang-tidy autofixes #15894

Apply clang-tidy autofixes #15894

Conversation

vyasr commented May 30, 2024

Description

Checklist

bdice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasr Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasr commented May 31, 2024

davidwendt commented May 31, 2024

vyasr commented May 31, 2024

davidwendt commented May 31, 2024

vyasr commented May 31, 2024

bdice left a comment • edited Loading

Choose a reason for hiding this comment

vyasr commented Jun 3, 2024

Choose a reason for hiding this comment

vyasr Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PointKernel commented Jun 4, 2024

vyasr commented Jun 5, 2024

srinivasyadav18 left a comment

Choose a reason for hiding this comment

ttnghia left a comment

Choose a reason for hiding this comment

vyasr commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyleFromNVIDIA commented Aug 27, 2024

vyasr commented Aug 27, 2024

vyasr Jun 3, 2024 •

edited

Loading

bdice left a comment •

edited

Loading

vyasr Jun 5, 2024 •

edited

Loading