-
Notifications
You must be signed in to change notification settings - Fork 38
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
Fix issues identified by the cudf clang-tidy run #662
Comments
@paleolimbot do you want me to keep rapidsai/cudf#17083 open, or should I go ahead and close it? I can keep the branch alive for as long as you want. I don't mean to rush you, just cleaning house a bit on my end. |
Sorry for being slow! I really am going to look at this 😬 Feel free to close the PR but if you don't mind keeping the branch open that would be helpful. (For my own brain: rapidsai/cudf@branch-24.12...vyasr:cudf:testing/nanoarrow_clang_tidy ). |
No rush at all! I was just cleaning up my open PRs list. I'll close the PR but leave the branch. |
I was going to try to reproduce this but have discovered that a driver update seems to have borked any ability to do GPU things today 🙁 I see that you're running clang-tidy in release mode, which is probably why none of my updates have helped: cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCUDF_CLANG_TIDY=ON -GNinja This doesn't have to be the permanent fix, but my working hypothesis is that adding In general, I think the issue is that we maintain certain invariants in the C runtime that aren't visible in the headers. We communicate that using DCHECK, but if you don't compile it with the debug checks, there's no way for clang-tidy to know about those invariants. In particular, the invariant I'm talking about is that if you reserve with |
Ah I see. Is |
Let me know if that works and/or is an acceptable solution! We could also make the public define something like |
@paleolimbot Sorry, lost track of this for a bit! Unfortunately it looks like setting NANOARROW_DEBUG doesn't seem to help. Am I missing something in rapidsai/cudf#17476? |
Kindly reproducible thanks to @vyasr (see #639 (comment) ). PR demonstrating the issue: rapidsai/cudf#17083
The traceback is similar to one that was "fixed" in our CI by adding
NANOARROW_DCHECK()
s to help clang-tidy (at least on our local run) see through some of the invariants we know about with respect to what happens on the "reserve" step (or maybe I'm reading this too quickly and clang-tidy has identified an issue).One traceback:
The text was updated successfully, but these errors were encountered: