Conversation
WalkthroughBumps project and example versions, adds interactive stdin handling to the example, adds a lobber derivation and devShell changes in flake.nix, centralizes builder validations into Build() with new OOR-style ErrorT values and removes Builder::IsValid(), and updates tests to assert explicit error codes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example/lobber.cpp (1)
1-737: CI pipeline failure: formatting issue detected.The pipeline reports that this file is badly formatted. Run the CMake lint formatter to fix this before merging.
🧹 Nitpick comments (2)
source/lob_builder.cpp (1)
516-519: Potential issue with velocity NaN check.The condition
std::isnan(pimpl->build.velocity)will never be true becausevelocityis auint16_t, which cannot represent NaN. The checkpimpl->build.velocity <= 0alone is sufficient since the default value is 0.Consider simplifying:
- if (std::isnan(pimpl->build.velocity) || pimpl->build.velocity <= 0) { + if (pimpl->build.velocity == 0) { pimpl->build.error = ErrorT::kInitialVelocityRequired; return; }flake.nix (1)
52-73: Consider adding error handling for the manual binary copy.The
installPhaseassumes the binary will be atbuild/example/lobber. While this path is correct per CMake's standard output structure (the example subdirectory creates binaries inbuild/example/), adding a check ensures the build didn't fail unexpectedly:installPhase = '' mkdir -p $out/bin + if [ ! -f build/example/lobber ]; then + echo "Error: lobber binary not found at build/example/lobber" + exit 1 + fi cp build/example/lobber $out/bin/ '';Note: The example does not define CMake install targets for the binary, so the
cmake --installapproach is not applicable here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CMakeLists.txt(1 hunks)example/CMakeLists.txt(1 hunks)example/lobber.cpp(5 hunks)flake.nix(5 hunks)include/lob/lob.hpp(1 hunks)source/lob_builder.cpp(12 hunks)test/source/lob_builder_test.cpp(24 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-17T12:55:26.522Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 25
File: include/lob/lob.hpp:123-130
Timestamp: 2025-06-17T12:55:26.522Z
Learning: In the lob ballistics library, when Input::step_size is set to 0 (the default), the solver automatically sets an appropriate step size internally rather than failing with an assertion error.
Applied to files:
example/lobber.cppCMakeLists.txttest/source/lob_builder_test.cpp
📚 Learning: 2025-04-15T13:11:22.842Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 15
File: source/calc.hpp:52-52
Timestamp: 2025-04-15T13:11:22.842Z
Learning: In lob, the `CalculateProjectileReferenceArea` function returns area in square inches (SqInT), but in some calculations like those in test/source/calc_test.cpp, this value is intentionally converted to square feet (SqFtT) when multiplied by air density in pounds per cubic foot (LbsPerCuFtT) to ensure proper unit cancellation.
Applied to files:
example/lobber.cpptest/source/lob_builder_test.cpp
🪛 GitHub Actions: Continuous Integration
example/lobber.cpp
[error] 1-1: CMake lint check failed: The following files are badly formatted: example/lobber.cpp. Run again with FIX=YES to fix these files.
🔇 Additional comments (22)
example/CMakeLists.txt (1)
8-8: Version bump is appropriate and consistent.The example project version is correctly incremented to 1.0.3, aligning with the broader PR updates.
CMakeLists.txt (1)
11-11: Root project version bump is correct.The main project version is properly incremented to 0.7.0. This aligns with the PR's versioning objectives and the synchronized example project update to 1.0.3.
example/lobber.cpp (4)
19-23: Platform-specific includes for terminal detection look correct.The conditional includes for
_WIN32(using<io.h>) and POSIX (using<unistd.h>) are appropriate for cross-platformisattydetection.
73-80: Cross-platform IsInteractive() implementation is correct.The function properly uses
_isatty(_fileno(stdin))on Windows andisatty(STDIN_FILENO)on POSIX systems to detect interactive terminals.
528-539: Consider addingstd::decreset after hex output for safety.The error value is printed in hex format, and while
std::decandstd::noshowbaseare restored on line 538, this is good practice. The implementation looks correct.
640-666: Input handling logic is well-structured with proper error handling.The flow correctly:
- Checks for interactive mode first and runs the prompt wizard
- Falls back to stdin JSON parsing in non-interactive mode when data is available
- Provides clear error messaging when no input is provided
One minor observation: the
std::cin.peek()check on line 648 is a reasonable way to detect available stdin data, though it may block briefly on some systems.include/lob/lob.hpp (1)
63-91: ErrorT enum refactoring improves error specificity.The new OOR-suffixed variants (e.g.,
kAirPressureOOR,kAltitudeOfBarometerOOR) provide clearer error semantics than the previous generic names. The addition ofkBallisticCoefficientRequired,kInitialVelocityRequired, andkZeroDataRequireddistinguishes between missing required data vs. out-of-range values, which is a good API design choice.The
kNotFormedsentinel value is appropriate for indicating an uninitialized/in-progress build state.source/lob_builder.cpp (5)
355-360: Range angle validation is correct.The check ensures range angle is strictly between -90 and 90 degrees, which makes physical sense for ballistic calculations.
376-394: Altitude validation logic is thorough.The lambda
is_altitude_validcleanly validates all three altitude values (firing site, barometer, thermometer) against the stratosphere limit. Early returns with specific error codes are appropriate.
504-510: BuildOpticHeight provides sensible default.The 1.5-inch default optic height is a reasonable assumption for typical rifle scopes.
555-578: Validation checks in BuildBoatright have inconsistent boundary conditions.The checks use different boundary conditions:
meplat_diameter_in < InchT(0)(allows zero)base_diameter_in <= InchT(0)(disallows zero)nose_length_in < InchT(0)(allows zero)tail_length_in < InchT(0)(allows zero)ogive_rtr < 0 || ogive_rtr > 1.0(allows boundary values)This appears intentional based on physical constraints (meplat, nose, and tail can be zero for flat-base bullets, but base diameter cannot), so the logic is correct.
821-865: Centralized Build() pipeline with early error returns is well-structured.The stepwise approach with error checking after each build phase is clear and maintainable. Using
kNotFormedas a sentinel to detect errors is an effective pattern.test/source/lob_builder_test.cpp (6)
132-139: Test correctly validates missing velocity error.The test properly verifies that
kInitialVelocityRequiredis returned when velocity is not set.
141-148: Test correctly validates missing BC error.Verifies
kBallisticCoefficientRequiredis returned when ballistic coefficient is missing.
150-157: Test correctly validates missing zero data error.Verifies
kZeroDataRequiredis returned when neither zero angle nor zero distance is provided.
362-365: ResetWorks test correctly validates reset behavior.The test now properly:
- Builds a valid input and confirms
kNoneerror- Calls
Reset().Build()separately and confirms error state is notkNoneThis aligns with the new error-driven validation model.
564-576: MachVsDragTableBadParamsIgnored test updated correctly.The test verifies that passing null pointers to
MachVsDragTableis silently ignored, and the subsequentBuild()fails withkBallisticCoefficientRequiredsince no BC was set. This is appropriate defensive behavior.
789-812: ReadmeExampleIsValid test provides good integration coverage.This test exercises the full builder API with realistic values and verifies the build succeeds. Good for catching regressions.
flake.nix (4)
126-127: LGTM! Good use of variables for maintainability.Extracting the file names into variables improves maintainability and makes the shell hook code clearer. The
sourceDirvariable correctly escapes the CMake variable reference to prevent premature expansion.Also applies to: 134-134
148-148: LGTM! Using CMake variable reference improves portability.Using
${sourceDir}instead of a hardcoded path makes the CMake preset more flexible and portable. This is a good improvement.
157-157: LGTM! Correct escaping for CMake environment variable syntax.The escaped dollar sign (
\$env{...}) ensures the environment variable reference is preserved as literal text in the JSON output, which CMake will then interpret correctly. This is the proper escaping for this context.
188-206: LGTM! Improved file creation logic with better feedback.The dual-file creation flow is clearer and more maintainable than the previous version. The status messages improve the developer experience by confirming which files were created or already exist. The
.clangdconfiguration correctly points to the dev build directory for compile commands.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
example/lobber.cpp (2)
52-52: Consider clarifying stdin usage in help text.The usage line now shows "stdin", but the help text doesn't clearly explain that stdin is only read in non-interactive mode (when input is piped or redirected). In interactive mode, the wizard is used instead. Consider adding a note to clarify this dual behavior.
Example clarification:
- std::cout << "Usage: lobber [options] stdin\n" + std::cout << "Usage: lobber [options] [< input.json]\n" << "Options:\n" << " --h, --help Show this help message\n" << " --v, --version Show version information\n" << " --json Output results to stdout in json format\n" << " --if=FILE Input json file containing data to use " "instead of user prompts\n" << " --of=FILE Output json file where data is saved for " "future use as an input file\n" + << "\n" + << "Note: When run interactively (terminal), a wizard prompts for input.\n" + << " When stdin is redirected, JSON data is read from stdin.\n" << "Example:\n"
641-666: LGTM! Well-structured interactive/non-interactive input flow.The input handling logic cleanly separates interactive mode (wizard-based) from non-interactive mode (stdin JSON). The error handling with try-catch for JSON parsing is appropriate, and the final check ensures the program doesn't proceed without data.
For additional robustness, consider checking the stream state before
peek():} else { - if (std::cin.peek() != std::char_traits<char>::eof()) { + if (std::cin.good() && std::cin.peek() != std::char_traits<char>::eof()) { try { std::cin >> json;This ensures the stream is in a valid state before attempting to read, though in typical non-interactive usage (piped input), this shouldn't be necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
example/lobber.cpp(5 hunks)source/lob_builder.cpp(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-17T12:55:26.522Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 25
File: include/lob/lob.hpp:123-130
Timestamp: 2025-06-17T12:55:26.522Z
Learning: In the lob ballistics library, when Input::step_size is set to 0 (the default), the solver automatically sets an appropriate step size internally rather than failing with an assertion error.
Applied to files:
example/lobber.cpp
📚 Learning: 2025-04-15T13:11:22.842Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 15
File: source/calc.hpp:52-52
Timestamp: 2025-04-15T13:11:22.842Z
Learning: In lob, the `CalculateProjectileReferenceArea` function returns area in square inches (SqInT), but in some calculations like those in test/source/calc_test.cpp, this value is intentionally converted to square feet (SqFtT) when multiplied by air density in pounds per cubic foot (LbsPerCuFtT) to ensure proper unit cancellation.
Applied to files:
example/lobber.cpp
🧬 Code graph analysis (1)
source/lob_builder.cpp (1)
source/calc.hpp (6)
CalculateMillerTwistRuleCorrectionFactor(152-157)CalculateMillerTwistRuleCorrectionFactor(152-153)CalculateMillerTwistRuleCorrectionFactor(159-162)CalculateMillerTwistRuleCorrectionFactor(159-160)CalculateMillerTwistRuleStabilityFactor(130-150)CalculateMillerTwistRuleStabilityFactor(130-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (macos-latest, static)
- GitHub Check: test (macos-latest, shared)
- GitHub Check: test (windows-latest, shared)
- GitHub Check: test (windows-latest, static)
- GitHub Check: test (ubuntu-latest, shared)
- GitHub Check: test (ubuntu-latest, static)
- GitHub Check: sanitize
- GitHub Check: coverage
🔇 Additional comments (13)
example/lobber.cpp (3)
19-23: LGTM! Clean cross-platform terminal detection.The platform-specific includes are correct for implementing terminal interactivity checks on both Windows and POSIX systems.
73-80: LGTM! Standard terminal interactivity detection.The implementation correctly uses platform-specific APIs to determine whether stdin is connected to an interactive terminal. This enables the program to behave appropriately for both interactive use and scripted/piped workflows.
528-540: LGTM! Error display aligns with PR objectives.The addition of an "Error" column that displays the error code in hexadecimal format is a good enhancement for debugging and validation. The formatting (hex with showbase) provides clarity, and the implementation properly resets the stream state after output.
source/lob_builder.cpp (10)
426-429: Past review concern has been resolved.The missing early return after setting
kHumidityOORhas been added on line 428, consistent with the validation pattern used throughout this function.
453-461: LGTM: Ballistic coefficient validation is thorough.The two-stage validation (presence check followed by range check) ensures robust error handling with explicit error codes.
486-491: LGTM: Wind heading validation is appropriate.Checking that wind heading falls within ±360° prevents out-of-range values while accommodating both positive and negative angle representations.
505-511: LGTM: Sensible default for optic height.Providing a 1.5-inch default when not specified is reasonable, and the placement in the build sequence ensures
optic_heightis available beforeBuildZeroAnglerequires it.
517-535: LGTM: Validation logic correctly distinguishes required vs. optional parameters.Initial velocity is mandatory (line 517), while diameter/length/mass are validated only if provided. The two-stage approach (positivity checks followed by NaN checks at line 537) ensures stability calculation proceeds only when all necessary inputs are available.
556-579: LGTM: Boatright parameter validations are physically sound.The distinction between parameters that can be zero (meplat, nose, tail) versus those that must be positive (base diameter) aligns with the physical constraints of projectile geometry. Range validation for ogive RtR ∈ [0, 1] is appropriate.
707-719: LGTM: Coriolis validation correctly handles optional coupled parameters.Requiring both azimuth and latitude to be present before validating ensures Coriolis calculations only proceed with complete data. The bounds (±360° for azimuth, ±90° for latitude) are geographically appropriate.
741-758: LGTM: Zero angle configuration supports two valid workflows.The branching logic cleanly handles both manual zero angle specification (with bounds validation) and automatic calculation from zero distance data. The explicit error codes (
kZeroAngleOOR,kZeroDataRequired,kZeroDistanceOOR) provide clear feedback for misconfiguration.
809-812: LGTM: Maximum time validation prevents infinite loops.Ensuring
max_time > 0is critical for preventing unbounded trajectory calculations. The explicit error code improves debuggability.
822-867: LGTM: Excellent error handling orchestration.The sentinel pattern (
kNotFormed→ early return →kNoneon success) provides clean, predictable error propagation. The build step ordering correctly respects dependencies:
BuildEnvironmentestablishes foundational values (air density, gravity)BuildTableuses air density from environmentBuildOpticHeightexecutes beforeBuildZeroAnglewhich depends on itBuildStabilityfeedsBuildBoatrightBuildZeroAngleuses outputs from earlier stepsThis centralized validation approach eliminates scattered checks in setters and makes error conditions explicit and testable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
source/lob_builder.cpp (2)
537-541: Silent return for optional stability factor parameters.If twist or any other stability-factor inputs are NaN, the function returns without setting an error. This is intentional behavior treating stability factor calculation as optional, but consider adding a comment to clarify this design decision for future maintainers.
823-856: Consider reducing error-check boilerplate.The repeated
if (pimpl_->build.error != ErrorT::kNotFormed) { return pimpl_->build; }pattern is correct but verbose. A helper lambda could reduce repetition:Input Builder::Build() { pimpl_->build.error = ErrorT::kNotFormed; + auto has_error = [this]() { + return pimpl_->build.error != ErrorT::kNotFormed; + }; // This order matters BuildEnvironment(pimpl_); - if (pimpl_->build.error != ErrorT::kNotFormed) { - return pimpl_->build; - } + if (has_error()) return pimpl_->build; BuildTable(pimpl_); - if (pimpl_->build.error != ErrorT::kNotFormed) { - return pimpl_->build; - } + if (has_error()) return pimpl_->build; // ... etc.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
example/lobber.cpp(6 hunks)flake.nix(5 hunks)source/lob_builder.cpp(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-17T12:55:26.522Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 25
File: include/lob/lob.hpp:123-130
Timestamp: 2025-06-17T12:55:26.522Z
Learning: In the lob ballistics library, when Input::step_size is set to 0 (the default), the solver automatically sets an appropriate step size internally rather than failing with an assertion error.
Applied to files:
example/lobber.cpp
📚 Learning: 2025-04-15T13:11:22.842Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 15
File: source/calc.hpp:52-52
Timestamp: 2025-04-15T13:11:22.842Z
Learning: In lob, the `CalculateProjectileReferenceArea` function returns area in square inches (SqInT), but in some calculations like those in test/source/calc_test.cpp, this value is intentionally converted to square feet (SqFtT) when multiplied by air density in pounds per cubic foot (LbsPerCuFtT) to ensure proper unit cancellation.
Applied to files:
example/lobber.cpp
🧬 Code graph analysis (1)
source/lob_builder.cpp (1)
source/calc.hpp (6)
CalculateMillerTwistRuleCorrectionFactor(152-157)CalculateMillerTwistRuleCorrectionFactor(152-153)CalculateMillerTwistRuleCorrectionFactor(159-162)CalculateMillerTwistRuleCorrectionFactor(159-160)CalculateMillerTwistRuleStabilityFactor(130-150)CalculateMillerTwistRuleStabilityFactor(130-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (ubuntu-latest, shared)
- GitHub Check: test (windows-latest, shared)
- GitHub Check: test (windows-latest, static)
- GitHub Check: test (macos-latest, static)
- GitHub Check: test (macos-latest, shared)
- GitHub Check: test (ubuntu-latest, static)
- GitHub Check: coverage
- GitHub Check: sanitize
🔇 Additional comments (21)
example/lobber.cpp (6)
13-13: LGTM!The
<map>include is correctly added and required forstd::mapusage inGetStateKeys()andGetStatePrompts().
19-23: LGTM!Standard cross-platform approach for terminal detection with appropriate platform-specific headers.
51-67: LGTM!Clear and informative help text documenting both interactive wizard mode and stdin JSON redirection.
76-83: LGTM!Correct cross-platform implementation for detecting interactive terminal sessions.
531-542: LGTM!Good addition of the error column for debugging. The hex formatting with
std::showbaseis appropriate for error codes, and formatting state is properly reset afterward.
643-669: LGTM!Well-structured control flow handling the three input modes (file, interactive wizard, stdin redirect) with appropriate error handling. The
std::cin.peek()check correctly detects available stdin data without consuming it.flake.nix (4)
130-131: Good improvements to configuration file handling.The new variables (lines 130-131) improve maintainability, and the updated
binaryDirpath (line 152) using thesourceDirvariable makes the configuration more flexible compared to the hard-coded path. The escaping for bothsourceDirandCMAKE_CXX_FLAGScorrectly preserves the CMake variable references in the generated JSON.Also applies to: 138-138, 152-152, 161-161
212-212: Good practice preserving the base shellHook.Appending
oldAttrs.shellHookensures that the base shell's hook logic is preserved, maintaining backward compatibility.
192-210: Well-structured file creation flow with helpful user feedback.The dual-file creation logic with existence checks and status messages provides a good user experience. The .clangd configuration format is correct, with CompileFlags: CompilationDatabase: path properly specified for locating the build directory containing compile_commands.json. The approach prevents accidental overwrites and clearly communicates what actions were taken.
52-77: Well-implemented error handling in the lobber derivation.The binary existence check (lines 71-74) provides clear error reporting before attempting to copy. The
nlohmann_jsondependency is correctly included—the example extensively uses it throughout the codebase and declares it as a required dependency in CMakeLists.txt.source/lob_builder.cpp (11)
355-360: LGTM - Range angle validation with proper early return.Validation correctly checks that range angle is within (-90°, 90°) and returns early on error.
376-394: Clean use of lambda for altitude validation.Good approach using a local lambda to avoid code duplication for the three altitude validations.
426-429: Humidity validation now includes early return.The previously flagged issue has been addressed.
453-461: Good separation of "required" vs "out of range" errors.Distinguishing between missing BC (
kBallisticCoefficientRequired) and invalid BC (kBallisticCoefficientOOR) provides clearer error reporting to users.
486-491: Wind heading validation looks correct.Allowing headings within ±360° is appropriate for angular input normalization.
505-511: LGTM - Sensible default for optic height.The 1.5-inch default is a reasonable scope height assumption when not explicitly provided.
556-579: Thorough validation for Boatright calculation inputs.The validation correctly distinguishes between parameters that can be zero (meplat diameter, nose length, tail length) and those that must be positive (base diameter). Ogive ratio bounded to [0, 1] is appropriate.
707-719: Correct Coriolis validation requiring both azimuth and latitude.The "both or neither" pattern is appropriate since Coriolis calculations require both values. Latitude bounds of ±90° are geographically correct.
741-758: Zero angle/distance validation logic is correct.The precedence is clear: if
zero_angleis explicitly provided, validate and use it; otherwise, require a validzero_distancefor calculation. The distinction betweenkZeroDataRequired(neither provided) andkZeroDistanceOOR(invalid distance) is helpful for user feedback.
822-861: Error orchestration pattern is correct.The sentinel value pattern (
kNotFormed→ phase errors →kNoneon success) works correctly. The check is appropriately omitted afterBuildOpticHeightandBuildLitzAerodynamicJumpsince they don't set errors.
814-817: The undefined behavior claim regarding NaN handling instd::maxis valid.If
minimum_energy_ft_lbsis NaN (the default), std::max requires a strict weak ordering relation between arguments, which NaN violates since NaN comparisons are all false. Additionally, converting NaN to integer types is undefined behavior, as integer types have no NaN representation.The suggested guard should be applied:
+ if (std::isnan(pimpl->minimum_energy_ft_lbs)) { + return; + } const FpsT kMinSpeed = CalculateVelocityFromKineticEnergy( pimpl->minimum_energy_ft_lbs, SlugT(LbsT(pimpl->build.mass))); pimpl->build.minimum_speed = std::max(pimpl->build.minimum_speed, kMinSpeed.U16());Alternatively, consider using std::fmax, which treats NaN as missing data and returns the other operand, though this would require adapting the code to work with FpsT.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.