Repository navigation
fix: clean compiler warnings without redundant initialization - #321
Merged
Merged
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (20)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
GCC Release and Clang Debug builds report repeated aggregate-initialization, borrowed-view, unused-value, and range-check warnings. Clean up these diagnostics while preserving intentionally uninitialized storage and avoiding extra writes on command paths.
Uses merged eloqdata/bycorf#14 at
8caaf75cb131b591922af51a19861f92e2ffc893. This mainline pin also includes the previously merged direct BSD-to-DPDK packet-copy optimization in eloqdata/bycorf#12.Behavior before and after
The checked Linux ARM64 GCC 13 Release and Clang 18 Debug builds now complete with zero compiler warnings. Command replies, recovery formats, and notification failure behavior remain unchanged. GCC's existing LTO ABI compatibility note remains.
Implementation
CompactValueUpdateandStreamGroupSummarywhere GCC otherwise introduces redundant stores.SortExecKey; retain its argument/name selection contract.size_toperands, remove unnecessary constexpr captures, and explicitly discard results in exception/death tests.read()diagnostic, keeping the output-only counter uninitialized with a nearby explanation.Design decisions and alternatives
No warning categories are disabled. Empty C++ object state remains initialized where callers rely on it; unused raw output storage stays uninitialized. Optimized assembly checks for seven representative aggregate constructions showed identical instructions under Clang; GCC was identical in six cases and removed stores in
StreamTrimResult. The Bycorf notification-drain warning fix generates identical optimized instructions; the separately merged transmit-copy optimization is outside that comparison. These are code-generation checks, not throughput benchmarks.Documentation and comments
Added comments explaining the no-initialization case, aggregate initialization/code-generation choice, selected recovery union member, and SORT key contract. No architectural claim, module boundary, durable format, or lifecycle changes, so architecture documentation is unchanged.
Test plan
All checks ran on Linux ARM64. Both build directories had
LAVIK_ENABLE_WERROR=ONandLAVIK_KERNEL_BYPASS=OFF;builduses GCC 13 Release with optimization, andbuild_ciuses Clang 18 Debug with fault injection.cmake --build build --target lavik lavik-meta lavik-ctl --parallel 4 cmake --build build_ci --parallel 4 python3 tests/runtime_backends_smoke.py build/lavik --expect-no-bypass ctest --test-dir build_ci -R '^(meta\.MetaAuditStore\.|RedisConfigTest\.|CowArray(Test|DeathTest)\.|HashGroupEdits\.|Grouped(Hash|Collection|SortedRewrite|OrderedObject|ObjectIndex)Test\.|CollectionCompactStream\.|Stream(PageMaxKey|Directory)Test\.|ListRingTest\.|Rdb(Test|CollectionTest|CollectionImportTest)\.|lavik_(version|runtime_backends|stream_records|rdb_backup_e2e)$|meta_integration\.gate_managed_sort$)' --output-on-failure --parallel 1Builds and Release runtime smoke passed. The CTest selection passed 257/258 tests; the SORT gate stopped at its prerequisite check because the system Redis was 7.0.15. Installed the repository-pinned peer and reran that gate successfully in both Single and Cluster modes:
Additional focused integration coverage passed (4 recovery cases and 10 ordered-collection cases):
The DPDK translation unit also passed GCC 13 and Clang 18 syntax checks with
_FORTIFY_SOURCE=3and-Werror, including at the merged Bycorf pin; details of the original warning check are in the dependency PR.At the final merged pin, both Lavik builds and the Release runtime smoke were checked again. The existing bypass-enabled Bycorf build also passed with zero warnings, followed by local-connect and TCP-sequence regression checks:
cmake --build /tmp/bycorf-direct-frame-build --target bycorf bycorf_freebsd_port_check bycorf_freebsd_local_connect_check bycorf_freebsd_tcp_sequence_check --parallel 4 ctest --test-dir /tmp/bycorf-direct-frame-build -R '^bycorf_freebsd_local_connect_check$' --output-on-failure /tmp/bycorf-direct-frame-build/bycorf_freebsd_tcp_sequence_checkRisk assessment
The main review points are recovery union selection and preserving SORT's EXEC/script key selection. Focused recovery and managed SORT tests cover both. Full x86_64 CI and hardware DPDK/SPDK execution were not run locally.
Rollback plan
Revert this PR, including the Bycorf gitlink.
Reviewer guide
Start with
src/storage/engine/recovery.cpp,src/redis/sort_command.h, andinclude/lavik/storage/engine.h; the remaining changes make initialization/discard intent explicit or remove warning-producing expression forms.Follow-up work
The Bycorf dependency is merged and the submodule pin references its mainline commit. No dependency follow-up remains.
Summary by CodeRabbit