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

tidy: add clang-tidy #2156

Merged
merged 1 commit into from
Oct 10, 2023
Merged

tidy: add clang-tidy #2156

merged 1 commit into from
Oct 10, 2023

Conversation

Riolku
Copy link
Contributor

@Riolku Riolku commented Oct 5, 2023

clang-tidy is a static analyzer, and can find many different issues. So far I have integrated a few simple checks, and disabled the remaining ones. They will be enabled in future PRs.

Closes #2152. Ref #2148.

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (abca5e6) 89.92% compared to head (159f612) 90.04%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2156      +/-   ##
==========================================
+ Coverage   89.92%   90.04%   +0.11%     
==========================================
  Files         989      989              
  Lines       35637    35449     -188     
==========================================
- Hits        32048    31919     -129     
+ Misses       3589     3530      -59     
Files Coverage Δ
src/common/arrow/arrow_converter.cpp 97.24% <ø> (+27.24%) ⬆️
src/common/types/date_t.cpp 89.62% <ø> (ø)
src/common/types/dtime_t.cpp 74.68% <ø> (ø)
src/common/types/types.cpp 92.25% <100.00%> (-0.02%) ⬇️
src/function/base_lower_upper_operation.cpp 100.00% <100.00%> (ø)
src/include/common/types/cast_helpers.h 87.14% <ø> (ø)
...include/function/arithmetic/arithmetic_functions.h 93.22% <100.00%> (+0.11%) ⬆️
src/include/function/boolean/boolean_functions.h 85.71% <100.00%> (-3.18%) ⬇️
src/include/processor/operator/scan_node_id.h 100.00% <ø> (ø)
src/main/plan_printer.cpp 92.22% <100.00%> (+0.04%) ⬆️
... and 14 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/function/base_lower_upper_operation.cpp Outdated Show resolved Hide resolved
src/main/plan_printer.cpp Outdated Show resolved Hide resolved
@ray6080
Copy link
Contributor

ray6080 commented Oct 7, 2023

Is there a summarized list of checks we plan to integrate later?

@Riolku
Copy link
Contributor Author

Riolku commented Oct 7, 2023

Is there a summarized list of checks we plan to integrate later?

Not really, I plan to integrate everything except stuff I think is dumb or for which the check is poor quality.

For instance, I disabled bugprone-exception-escape because of the poor quality of the errors. I disabled bugprone namespace forward declarations because I dont see how this could be an issue, and we had some warnings from it and Arrow.

I disabled the widening multiplication becausr there are a lot of errors, and we need to fix that in its own PR.

@Riolku
Copy link
Contributor Author

Riolku commented Oct 10, 2023

Remaining todo: add tests for the bugs I fixed.
Also, we should make clangd have its own directory to avoid conflicts with release.

@Riolku Riolku force-pushed the clang-tidy branch 6 times, most recently from d45a96f to acf3a19 Compare October 10, 2023 19:10
clang-tidy is a static analyzer, and can find many different issues.
So far I have integrated a few simple checks, and disabled the remaining
ones. They will be enabled in future PRs.

Closes #2152. Ref #2148.
@Riolku
Copy link
Contributor Author

Riolku commented Oct 10, 2023

Note: we can't easily test returning node and getting the arrow schema of the result because we don't have good enough data type support for getArrowSchema and the nodes in tinysnb all have incompatible data types.

@Riolku Riolku merged commit 3aae729 into master Oct 10, 2023
10 of 11 checks passed
@Riolku Riolku deleted the clang-tidy branch October 10, 2023 20:57
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.

Segfault in getArrowSchema
4 participants