-
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
chore(ci): Add clang-tidy checks and ensure they pass #639
Conversation
Still need to fix some additional decimal checks, possibly related to #594.
|
@vyasr I'm not seeing anything from buffer_inline...any chance you could share the full output from those errors? I am thinking that it may be related to your specific usage (that might be incorrect, or might be exposing a code path we didn't consider). |
Yeah one of those sounds quite likely. I've attached the output of running clang-tidy on one of our test files without patching in some NOLINT markers. |
r/.clang-tidy
Outdated
@@ -0,0 +1,21 @@ | |||
# Licensed to the Apache Software Foundation (ASF) under one |
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.
Do you mean for this file to be in the r directory or in the project root?
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.
😬 (Thanks!)
Thank you! I'll plug through more details tomorrow but I am wondering if clang-tidy can't see that if For my brain tomorrow, there is:
...and I wonder if
|
Here's another error in our CI (one that I haven't reproduced locally yet). This one may be our fault if there's some function that isn't safe to call when a buffer is empty, but I'm just letting you know in case it helps you track anything down. I haven't traced it yet. |
0c330c6
to
beaef47
Compare
It seems like clang-tidy has some trouble guessing whether or not an allocation does or does not take place if the value of |
Yeah I would expect assertions to provide useful hints since other linters certainly work that way. When you have something working reasonably well let me know and I can test it directly within our repo by repointing the local clone to use this branch easily enough. |
@vyasr I think this is ready...a test from your end would be very helpful! (Also because we're about to release and it would be great to identify any issues before we get to RC0!) |
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.
lgtm
return; | ||
} | ||
|
||
NANOARROW_DCHECK(bits != NULL); |
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.
Since bits
is a parameter this might also be a good use case for the clang/gcc nonnull attribute
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 didn't know about that! I think it can be NULL if length == 0
here but there are other places where it could be useful!
Thanks for getting this done @paleolimbot! Unfortunately I do still see some clang-tidy errors coming out of our build after upgrading to use this commit. I'm going to try and get our clang-tidy setup working first so that it's easier to reproduce, and then I'll ping you again so that you don't have to jump through so many hoops (and of course I can also dig in again). |
Feel free to post any version of logs here in the meantime! |
I'm focusing on getting clang-tidy running in our CI and easy to reproduce locally with pre-commit before I go any further here, that way I'll be able to hand it off to others like you without you having to recreate unnecessary complexity. So it'll probably be a week or so before I have something ready there, but once that happens I'll hopefully be able to post logs and also do so in a way that you can easily reproduce without needing to fully recompile our software or anything. |
Ah ok! I am planning to put up a release candidate up on Monday (but can obviously follow up with fixes after). I don't personally mind unreproducible logs but no pressure! |
I see that the release went up, nice! I just merged the PR to add clang-tidy to our CI in a way that should make it pretty easy to reproduce our issues locally now. Here's a PR that should demonstrate the issue. All you need to reproduce is in the edited Here's the traceback.
|
Thank you! I opened a new issue to track so that this doesn't get lost. I'll take a look this week! |
Thanks! Let me know if you need help with the repro. |
This PR adds a
clang-tidy
check to CI and fixes several issues that it identified (including a few from other repos like ADBC and cudf).A reboot of #538; closes #537.