Clear the compiler warnings in BGEN's own sources - #6
Conversation
Upstream 1.1.7 emits 45 warnings under -Wall -pedantic with GCC 13, 23 of them in BGEN's own sources, which buries anything new in the noise. - bgen.hpp: spell the allele count uint32_t( pack.numberOfAlleles ) in the phased-parsing path, matching the idiom used a few lines above. numberOfAlleles is a uint16_t, so 'numberOfAlleles - 1' promoted to int and the comparison converted it straight back to unsigned; the cast makes explicit what was already happening. This one site accounts for 18 of the 23, instantiated across translation units. - View.cpp: compare gcount(), a signed streamsize, with a size_t explicitly. - bgenix.cpp: valueSize is only compared against std::string::size(), so declare it std::size_t. - test_variant_data_block.cpp: catch BGenError by const reference rather than by value, which was slicing a polymorphic type. - 3rd_party: build vendored boost, sqlite3 and zstd with -w rather than chasing individual -Wno- flags for code we do not maintain. Every change is a type or spelling change. bgen_to_vcf produces byte-identical output for all 68 files in example/ before and after, and the unit tests are unchanged at 1950006 assertions. A clean build goes from 45 warnings to 11, none of them in our own sources -- the rest are boost headers included by our translation units, which a flag on the boost target cannot reach. This revives the work from #3, re-indented with tabs to match the surrounding files, so the diff against upstream shows the tokens that actually changed. AGENTS.md is rewritten rather than restored, since the original predated the Python packaging and test suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JBPj5da6i7reenMxoSdyjW
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe changes fix several compiler-warning and type issues, suppress warnings from vendored dependencies, update tests and patch notes, advance the package version to ChangesWarning cleanup and packaging
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 10-13: Update all four shell command fences in AGENTS.md to
include a shell language identifier on each opener, using sh or console
consistently while preserving the existing commands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d301b18-8cf4-4e10-9c89-f4e98d7f35a8
📒 Files selected for processing (10)
3rd_party/boost_1_55_0/wscript3rd_party/sqlite3/wscript3rd_party/zstd-1.1.0/wscriptAGENTS.mdPATCHES.mdapps/bgenix.cppgenfile/include/genfile/bgen/bgen.hpppyproject.tomlsrc/View.cpptest/unit/test_variant_data_block.cpp
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JBPj5da6i7reenMxoSdyjW
1 participant
Revives the work from #3 (and its duplicate #2), re-indented and verified.
Upstream 1.1.7 emits 45 warnings under
-Wall -pedanticwith GCC 13, 23 of themin BGEN's own sources, which buries anything new in the noise.
genfile/include/genfile/bgen/bgen.hpptest/unit/test_variant_data_block.cppsrc/View.cppapps/bgenix.cppChanges
bgen.hpp— spell the allele countuint32_t( pack.numberOfAlleles )inthe phased-parsing path, matching the idiom already used a few lines above.
numberOfAllelesis auint16_t, sonumberOfAlleles - 1promoted tointand the comparison converted it straight back to unsigned; the cast makes
explicit what was already happening. This single site accounts for 18 of the
23 warnings, instantiated across translation units.
View.cpp— comparegcount(), a signedstd::streamsize, with astd::size_texplicitly.bgenix.cpp—valueSizeis only ever compared againststd::string::size(), so declare itstd::size_t.test_variant_data_block.cpp— catchBGenError const&rather than byvalue in
REQUIRE_THROWS_AS, which was slicing a polymorphic type.3rd_party/*/wscript— build the vendored libraries with-winstead ofchasing individual
-Wno-flags for code we do not maintain.Verification
Every change is a type or spelling change, so this should be invisible at
runtime — and is:
bgen_to_vcfproduces byte-identical output for all 68 files inexample/, comparing a build ofmainagainst this branch.bgenix -listoutput is identical forexample.16bits.bgen,haplotypes.bgenandcomplex.bgen, the last two covering the phased andmultiallelic paths that the
bgen.hppchange touches.Notes
whole-line rewrites. These are re-indented with tabs, leaving a diff that
shows the tokens that actually changed.
AGENTS.mdis rewritten rather than restored — the version on Fix compiler warnings #3 predated thePython packaging and pytest suite.
units, which a flag on the boost target cannot reach. Building those includes
with
-isystemwould clear them; that is a larger change to how include pathspropagate, so it is left out here.
1.1.7.post2andPATCHES.mdrecords the new deltas, per thescheme in the README. Nothing publishes until a
v1.1.7.post2tag is pushed.🤖 Generated with Claude Code
https://claude.ai/code/session_01JBPj5da6i7reenMxoSdyjW
Summary by CodeRabbit
Bug Fixes
Build Improvements
Documentation