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

test(c/driver): fix clang-tidy warnings #1928

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Conversation

lidavidm
Copy link
Member

Fixes #1927.

@lidavidm lidavidm added this to the ADBC Libraries 13 milestone Jun 20, 2024
@lidavidm lidavidm marked this pull request as ready for review June 20, 2024 02:40
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Sorry for taking a few days to get back here!

I think the actual errors might be solved by ensuring that the correct standard library is installed? I see that the first google result here is one of your blog posts, but perhaps the symlink fix isn't working anymore (or is still picking up the wrong -std library?)

@paleolimbot
Copy link
Member

It doesn't look like it's that easy to ignore directories, but the "right" way to do it might be to run clang-tidy in nanoarrow and perhaps ignore the nanoarrow headers here (or if warnings are OK, just live with the warnings for now). The nanoarrow repo is undergoing lots of general tidying at the moment and I can make sure clang-tidy is a part of that ( apache/arrow-nanoarrow#537 ).

@lidavidm
Copy link
Member Author

It doesn't look like it's that easy to ignore directories, but the "right" way to do it might be to run clang-tidy in nanoarrow and perhaps ignore the nanoarrow headers here (or if warnings are OK, just live with the warnings for now). The nanoarrow repo is undergoing lots of general tidying at the moment and I can make sure clang-tidy is a part of that ( apache/arrow-nanoarrow#537 ).

Yeah, I was trying to figure out how to just ignore Nanoarrow. I suppose we could invoke jq to remove it from the compilation database... 😬

I think the actual errors might be solved by ensuring that the correct standard library is installed? I see that the first google result here is one of your blog posts, but perhaps the symlink fix isn't working anymore (or is still picking up the wrong -std library?)

D'oh...

@paleolimbot
Copy link
Member

Yeah, I was trying to figure out how to just ignore Nanoarrow.

I gave it a try but I'm having the same issue locally with the standard library (C seems to work but not C++)!

I started apache/arrow-nanoarrow#538 and will poke away at it (clang-tidy is awesome and caught quite a few things).

@lidavidm lidavidm requested a review from zeroshade as a code owner June 25, 2024 01:37
@lidavidm lidavidm merged commit c1ad8df into apache:main Jun 25, 2024
58 of 64 checks passed
@lidavidm lidavidm deleted the gh-1927 branch June 25, 2024 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

c: fix clang-tidy warnings
2 participants