sre tidy changes #3108
sre tidy changes #3108
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces clang-tidy static analysis to the codebase and addresses numerous issues identified by the tool. The changes focus on fixing compilation warnings and improving code quality through explicit type casting, variable initialization, and better code practices.
Key changes include:
- Adding clang-tidy configuration and integration into the build workflow
- Fixing implicit type conversions with explicit static_cast operations
- Initializing variables to prevent undefined behavior
- Replacing magic numbers with named constants
- Adding proper initialization to RAII guard classes
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| vcpkg | Added vcpkg submodule reference for dependency management |
| scripts/run_clang_tidy.sh | New script to run clang-tidy analysis in parallel across source files |
| scripts/build_scripts/manage_cache.sh | Fixed architecture detection to use uname -m instead of arch |
| cpp/deeplake_pg/.clang-tidy | Configuration file defining clang-tidy rules and naming conventions |
| cpp/deeplake_pg/utils.hpp | Added explicit casts and variable initializations to fix type safety issues |
| cpp/deeplake_pg/table_version.cpp | Made RAII locker class non-copyable/movable and added explicit cast |
| cpp/deeplake_pg/table_scan_impl.hpp | Changed loop variables to explicit int32_t type |
| cpp/deeplake_pg/table_ddl_lock.cpp | Added explicit cast and NOLINT comment for global variable |
| cpp/deeplake_pg/table_data_impl.hpp | Added extensive explicit casts for array indexing and size conversions |
| cpp/deeplake_pg/table_am.cpp | Added named constants, explicit casts, and proper struct initialization |
| cpp/deeplake_pg/pg_to_duckdb_translator.cpp | Fixed isspace/tolower calls and simplified boolean logic |
| cpp/deeplake_pg/pg_deeplake.cpp | Introduced named constants for column indices and thread pool configuration |
| cpp/deeplake_pg/nd_utils.hpp | Added explicit casts and variable initializations throughout array operations |
| cpp/deeplake_pg/memory_tracker.cpp | Replaced magic numbers with named constants for byte conversions |
| cpp/deeplake_pg/hybrid_query_merge.hpp | Added explicit casts for array indexing operations |
| cpp/deeplake_pg/extension_init.cpp | Initialized ListCell pointer to nullptr |
| cpp/deeplake_pg/duckdb_pg_convert.cpp | Added explicit casts for memory allocation sizes |
| cpp/deeplake_pg/duckdb_deeplake_scan.cpp | Added explicit casts for memcpy size calculations |
| cpp/deeplake_pg/duckdb_deeplake_convert.cpp | Added explicit cast for size calculation |
| cpp/deeplake_pg/deeplake_executor.cpp | Added proper struct initialization and explicit casts |
| Taskfile.yml | New task automation configuration for building, testing, and linting |
| .github/workflows/pg-extension-build.yaml | Added clang-tidy step to CI workflow (with continue-on-error) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local level ts msg | ||
| level="$1" | ||
| ts="$(date --utc -Iseconds)" | ||
| shift |
There was a problem hiding this comment.
The log function does not use the provided arguments correctly. The shift command removes the level argument, but then the function tries to print ${msg} which is never assigned. The remaining arguments after shift should be concatenated into msg or the printf statement should use "$*" to print all remaining arguments.
| shift | |
| shift | |
| msg="$*" |
| worker_count="$(nproc)" | ||
| for file in *.cpp; do | ||
| ((counter++)) | ||
| run_tidy $file & |
There was a problem hiding this comment.
The variable $file should be quoted to handle filenames with spaces correctly. Use run_tidy "$file" & instead.
| run_tidy $file & | |
| run_tidy "$file" & |
scripts/run_clang_tidy.sh
Outdated
| done | ||
| wait | ||
|
|
||
| log "processing results..." |
There was a problem hiding this comment.
The log function call is missing the level argument. It should be log info "processing results..." to match the function signature.
| log "processing results..." | |
| log info "processing results..." |
| for (int32_t i = 0; i < table_data_.num_columns(); ++i) { | ||
| } |
There was a problem hiding this comment.
This loop body is empty and serves no purpose. If this is intentional placeholder code, add a comment explaining why. Otherwise, this loop should be removed.
| for (int32_t i = 0; i < table_data_.num_columns(); ++i) { | |
| } |
| unit = "seconds"; | ||
| } else { | ||
| unit = "seconds"; // default | ||
| unit = "seconds"; // default for "1" and other cases |
There was a problem hiding this comment.
The comment mentions handling "1" and other cases, but the code only reaches this else branch when multiplier is not "1000", "1000000", or other specific values. The previous condition else if (multiplier == "1") was removed. If multiplier "1" should map to "seconds", it should be explicitly checked, or the comment should be updated to reflect that this is truly a default fallback.
| for (char& ch : next_keyword_upper) { | ||
| ch = static_cast<char>(std::toupper(static_cast<unsigned char>(ch))); | ||
| } |
There was a problem hiding this comment.
This pattern of converting to uppercase appears multiple times in this function (lines 690-692, 709-711). Consider extracting this into a helper function to reduce code duplication.
| mkdir -p "$EXT_DIR" | ||
| cp postgres/*.control "$EXT_DIR/" 2>/dev/null || true | ||
| cp postgres/*.sql "$EXT_DIR/" 2>/dev/null || grep -v "utils.psql" || true |
There was a problem hiding this comment.
The pipeline logic appears incorrect. The grep -v "utils.psql" will filter stdin (which is empty after cp fails), not the files being copied. This should likely be cp $(ls postgres/*.sql | grep -v "utils.psql") "$EXT_DIR/" 2>/dev/null || true if the intent is to exclude utils.psql.
| cp postgres/*.sql "$EXT_DIR/" 2>/dev/null || grep -v "utils.psql" || true | |
| cp $(ls postgres/*.sql 2>/dev/null | grep -v "utils.psql") "$EXT_DIR/" 2>/dev/null || true |
|
🚀 🚀 Pull Request
Impact
Description
Things to be aware of
Things to worry about
Additional Context