Introduce clang tidy and fix part of the issues. #3104
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces clang-tidy static analysis tooling to the project and addresses various code quality issues identified by the analyzer. The changes focus on improving type safety, code clarity, and adherence to modern C++ best practices.
Key Changes:
- Added clang-tidy configuration and automation script for continuous static analysis
- Fixed C-style casts by replacing them with static_cast for improved type safety
- Introduced named constants to replace magic numbers throughout the codebase
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/run_clang_tidy.sh | New bash script to automate clang-tidy execution across C++ source files |
| cpp/deeplake_pg/.clang-tidy | Configuration file defining clang-tidy rules and naming conventions |
| .github/workflows/pg-extension-build.yaml | CI/CD integration for running clang-tidy during builds |
| cpp/deeplake_pg/table_version.cpp | Added deleted copy/move constructors and replaced C-style cast |
| cpp/deeplake_pg/table_ddl_lock.cpp | Added NOLINT comment and replaced C-style cast |
| cpp/deeplake_pg/table_am.cpp | Replaced magic numbers with constants, fixed member initialization order, added deleted constructors |
| cpp/deeplake_pg/pg_to_duckdb_translator.cpp | Fixed isspace usage, replaced magic numbers with constants, improved control flow readability |
| cpp/deeplake_pg/pg_deeplake.cpp | Replaced magic column indices with named constants |
| cpp/deeplake_pg/memory_tracker.cpp | Introduced constants for byte conversion factors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| unit = "seconds"; // default | ||
| unit = "seconds"; // default for "1" and other cases | ||
| } |
There was a problem hiding this comment.
The removed explicit handling for multiplier == "1" is now implicitly handled in the else clause. Consider adding an explicit case for "1" to make the logic clearer and more maintainable, or update the comment to explicitly mention that "1" is the expected default case.
| } | ||
| if (is_or) { |
There was a problem hiding this comment.
The change from 'else if' to separate 'if' statements alters the control flow logic. If both is_and and is_or could theoretically be true simultaneously (though unlikely), this change could introduce a bug. Consider keeping the 'else if' structure unless there's a specific reason for the change.
|
🚀 🚀 Pull Request
Impact
Description
Things to be aware of
Things to worry about
Additional Context