Conversation
|
Caution Review failedFailed to post review comments. Configuration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🪛 LanguageToolCONTRIBUTING.md[typographical] ~5-~5: Consider isolating this adverbial phrase with commas. (IN_FACT_COMMA_2) [uncategorized] ~5-~5: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA) [grammar] ~16-~16: Did you mean “fewer”? The noun “characters” is countable. (FEWER_LESS) 🔇 Additional comments (1)
WalkthroughThis update introduces comprehensive input validation and error reporting to the builder system, adds explicit enumeration values and error codes, and implements a reset capability for the builder. Constants are updated, and extensive unit tests are added to verify error handling. Minor logic and formatting adjustments are made throughout. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Builder
participant Input
User->>Builder: Set parameters (e.g., altitude, velocity, etc.)
Builder->>Builder: Validate each parameter
alt Invalid parameter
Builder->>Builder: Set error code in Input
end
User->>Builder: Build()
Builder->>Input: Return Input with error code (if any)
User->>Builder: Reset()
Builder->>Builder: Clear internal state and error code
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
include/lob/lob.hpp (3)
49-60: Consider a zero/none sentinel forClockAngleTThe enum now starts at
1. Calling code that default-constructs aClockAngleTwill get the invalid value0, yet there is no symbolic constant to express that state.
AddingkNone = 0U(or similar) would make intent explicit and avoid “magic zero”.
63-86:ErrorTordering – keep “success” firstPlacing
kNonefirst is great. For maintainability consider forcing the sentinel to0Uexplicitly and leaving the rest un-numbered unless specific wire-protocol values are required:-enum class LOB_EXPORT ErrorT : uint8_t { - kNone, +enum class LOB_EXPORT ErrorT : uint8_t { + kNone = 0U,This prevents accidental re-ordering from changing the “success” code.
434-439:Reset()should benoexceptRebuilding the internal
Implshould never propagate, and marking the functionnoexceptallows the compiler to optimise move/copy operations.-Builder& Reset(); +Builder& Reset() noexcept;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CMakeLists.txt(1 hunks)include/lob/lob.hpp(11 hunks)source/constants.hpp(1 hunks)source/lob_builder.cpp(11 hunks)source/lob_solve.cpp(2 hunks)source/solve_step.cpp(2 hunks)test/source/lob_builder_test.cpp(1 hunks)test/source/lob_cwaj_test.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
source/solve_step.cpp (1)
Learnt from: joelbenway
PR: joelbenway/lob#22
File: source/lob_builder.cpp:520-552
Timestamp: 2025-05-22T16:06:12.031Z
Learning: The `SolveStep` function in `solve_step.hpp` has an overload with a default argument of `FeetT(1)` for the step parameter: `void SolveStep(TrajectoryStateT* ps, SecT* pt, const Input& input, FeetT step = FeetT(1))`
source/lob_solve.cpp (1)
Learnt from: joelbenway
PR: joelbenway/lob#22
File: source/lob_builder.cpp:520-552
Timestamp: 2025-05-22T16:06:12.031Z
Learning: The `SolveStep` function in `solve_step.hpp` has an overload with a default argument of `FeetT(1)` for the step parameter: `void SolveStep(TrajectoryStateT* ps, SecT* pt, const Input& input, FeetT step = FeetT(1))`
🧬 Code Graph Analysis (2)
source/lob_solve.cpp (2)
source/solve_step.cpp (4)
SolveStep(19-55)SolveStep(19-19)SolveStep(57-65)SolveStep(57-57)source/solve_step.hpp (2)
SolveStep(13-13)SolveStep(15-16)
source/lob_builder.cpp (2)
source/eng_units.hpp (4)
isnan(171-173)isnan(171-171)isnan(911-913)isnan(911-911)include/lob/lob.hpp (7)
InitialVelocityFps(258-258)MachVsDragTable(229-230)MachVsDragTable(241-244)MachVsDragTable(241-242)WindHeadingDeg(364-364)ZeroDistanceYds(289-289)Reset(438-438)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (windows-latest, static)
- GitHub Check: test (windows-latest, shared)
- GitHub Check: test (ubuntu-latest, static)
- GitHub Check: test (ubuntu-latest, shared)
- GitHub Check: sanitize
- GitHub Check: coverage
🔇 Additional comments (4)
source/constants.hpp (1)
20-20: Addition looks correctValue, naming, and digit-separator style are consistent with the other ISA constants.
No further action required.include/lob/lob.hpp (1)
27-33: Good move to explicit enumerator valuesExplicitly mapping each
DragFunctionTentry to its drag-function number removes guesswork when serialising/deserialising. 👍CMakeLists.txt (1)
11-11: Version bump acknowledgedProject version updated to 0.6.5 – matches the public API changes introduced in this PR.
test/source/lob_builder_test.cpp (1)
350-364: Add a test forRelativeHumidityPercentvalidationGiven the new validation, a case with an invalid humidity (e.g.
0or negative) should assertkRelativeHumidity. This both exercises the branch and would have caught the wrong enum referenced in the implementation.
| double stability_factor{NaN()}; /// @brief Miller stability factor. | ||
| double aerodynamic_jump{NaN()}; /// @brief Aerodynamic jump effect in Moa. | ||
| double spindrift_factor{NaN()}; /// @brief Spin drift factor. | ||
| uint16_t minimum_speed{0}; /// @brief Minimum speed for solver. | ||
| double max_time{NaN()}; /// @brief Max time of flight for solver. | ||
| uint16_t step_size{0}; /// @brief Step size for solver. | ||
| ErrorT error{ErrorT::kNotFormed}; /// @brief Builder error field. | ||
| }; // struct Input |
There was a problem hiding this comment.
Default step_size == 0 conflicts with solver assertions
SolveStep asserts that the step argument is non-zero (> 0.0 / != 0).
Input::step_size defaults to 0, which means that a caller who forgets to set a step will hit an assertion at runtime rather than receive a builder error.
Recommendation:
-uint16_t step_size{0};
+uint16_t step_size{1}; // safe minimumand/or make the builder set error = ErrorT::kStepSize when StepSize(…) is never invoked.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| double stability_factor{NaN()}; /// @brief Miller stability factor. | |
| double aerodynamic_jump{NaN()}; /// @brief Aerodynamic jump effect in Moa. | |
| double spindrift_factor{NaN()}; /// @brief Spin drift factor. | |
| uint16_t minimum_speed{0}; /// @brief Minimum speed for solver. | |
| double max_time{NaN()}; /// @brief Max time of flight for solver. | |
| uint16_t step_size{0}; /// @brief Step size for solver. | |
| ErrorT error{ErrorT::kNotFormed}; /// @brief Builder error field. | |
| }; // struct Input | |
| double stability_factor{NaN()}; /// @brief Miller stability factor. | |
| double aerodynamic_jump{NaN()}; /// @brief Aerodynamic jump effect in Moa. | |
| double spindrift_factor{NaN()}; /// @brief Spin drift factor. | |
| uint16_t minimum_speed{0}; /// @brief Minimum speed for solver. | |
| double max_time{NaN()}; /// @brief Max time of flight for solver. | |
| uint16_t step_size{1}; /// @brief Step size for solver (safe minimum). | |
| ErrorT error{ErrorT::kNotFormed}; /// @brief Builder error field. | |
| }; // struct Input |
🤖 Prompt for AI Agents
In include/lob/lob.hpp around lines 123 to 130, the default value of step_size
is set to 0, which conflicts with solver assertions that require step_size to be
greater than zero. To fix this, change the default step_size to a positive
non-zero value or modify the builder logic to set the error field to
ErrorT::kStepSize if StepSize(…) is never called, ensuring that missing step
size is caught as a builder error rather than causing a runtime assertion
failure.
There was a problem hiding this comment.
the solver sets the step size if it's zero at the time of solving
There was a problem hiding this comment.
@joelbenway Thank you for the clarification! I appreciate you explaining that the solver automatically handles the zero step_size case by setting an appropriate value internally. This makes my concern about assertion failures moot, as the solver has built-in logic to deal with this scenario.
I'll note this behavior for future reference.
🐰
◕ ◡ ◕
(Learning!)
✏️ Learnings added
Learnt from: joelbenway
PR: joelbenway/lob#25
File: include/lob/lob.hpp:123-130
Timestamp: 2025-06-17T12:55:26.513Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| assert(step.Value() > 0.0 && "step is not valid"); | ||
| const CartesianT<FpsT> kWind(FpsT(input.wind.x), FpsT(0.0), |
There was a problem hiding this comment.
Assertion contradicts new “negative step allowed” contract
lob_solve.cpp was changed to allow negative step sizes (step_size != 0), yet both overloads here still assert step.Value() > 0.0.
This will abort execution for any negative step passed through the new path.
Fix:
-assert(step.Value() > 0.0 && "step is not valid");
+assert(step.Value() != 0.0 && "step must be non-zero");Apply to both overloads (lines 22-23 and 60-61).
Also applies to: 60-61
🤖 Prompt for AI Agents
In source/solve_step.cpp at lines 22-23 and 60-61, the assertion incorrectly
requires step.Value() to be greater than 0, contradicting the new contract that
allows negative step sizes as long as step_size is not zero. Update both
assertions to check that step.Value() is not equal to 0 instead of being
strictly positive, ensuring negative steps are accepted without aborting
execution.
There was a problem hiding this comment.
step_size is a uint16_t so it could never be negative. The way it is communicates best.
| if (in.step_size != 0) { | ||
| SolveStep(&s, &t, in, SecT(UsecT(in.step_size))); | ||
| } else { | ||
| SolveStep(&s, &t, in, FeetT(1)); | ||
| SolveStep(&s, &t, in); | ||
| } |
There was a problem hiding this comment.
in.step_size must remain positive – current guard permits negatives
SolveStep asserts step.Value() > 0.0; switching the check from > 0 to != 0 lets a negative step_size propagate, tripping that assertion (or producing undefined physics in non-assert builds).
- if (in.step_size != 0) {
+ if (in.step_size > 0) {Either keep the original > 0 guard or add explicit sanitisation (e.g., std::abs) before converting to SecT.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (in.step_size != 0) { | |
| SolveStep(&s, &t, in, SecT(UsecT(in.step_size))); | |
| } else { | |
| SolveStep(&s, &t, in, FeetT(1)); | |
| SolveStep(&s, &t, in); | |
| } | |
| if (in.step_size > 0) { | |
| SolveStep(&s, &t, in, SecT(UsecT(in.step_size))); | |
| } else { | |
| SolveStep(&s, &t, in); | |
| } |
🤖 Prompt for AI Agents
In source/lob_solve.cpp around lines 89 to 93, the condition checking
in.step_size uses '!= 0', which allows negative values to pass through and cause
assertion failures in SolveStep. Change the condition to check 'in.step_size >
0' to ensure only positive step sizes are used, or alternatively sanitize the
value by applying std::abs before passing it to SecT to guarantee a positive
step size.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
source/lob_builder.cpp (1)
242-257:MachVsDragTable()may read past caller-supplied buffersYou only guard against
size == 0, yet the loop is hard-coded tokTableSize.
If the caller passes a table shorter thankTableSizethe code will read out-of-bounds, leading to UB.- for (size_t i = 0; i < kTableSize; i++) { + if (size < kTableSize) { + pimpl_->build.error = ErrorT::kMachDragTable; + return *this; + } + for (size_t i = 0; i < kTableSize; ++i) {At a minimum, assert
size >= kTableSizeor iterate up tosize.
The existing unit test always passes a full-length table, so this slipped through.
🧹 Nitpick comments (5)
source/lob_builder.cpp (1)
361-369: Minor: simplify wind-heading normalisationThe
if/elsecan be collapsed withstd::fmod(or manual mod) for slightly clearer intent:- if (kPosition.Value() > 0) { - pimpl_->wind_heading_rad = kDegreesPerClockNumber * kPosition; - } else { - pimpl_->wind_heading_rad = - kDegreesPerClockNumber * kPosition + kDegreesPerTurn; - } + pimpl_->wind_heading_rad = + std::fmod(kDegreesPerClockNumber * kPosition + kDegreesPerTurn, + kDegreesPerTurn);Optional, readability only.
test/source/lob_builder_test.cpp (2)
350-364:ResetWorksasserts wrong post-reset stateAfter
Reset()no inputs are set, so the nextBuild()call first coerces
errorfromkNone→kNotFormed, then fails validation and returns
immediately.
Your test expects this (kNotFormed), but the exercised path never verifies
that the previous inputs were actually cleared.
Consider asserting that previously-set fields (e.g.velocity) are NaN/zero to
prove the reset was effective.
350-728: Heavy duplication across error tests – extract a helperEvery error test repeats ~10 identical “happy-path” setters before tweaking one
parameter. This inflates the file and complicates maintenance.Input BuildBaseline(Builder* b) { return b->BallisticCoefficientPsi(0.436) .BCAtmosphere(AtmosphereReferenceT::kArmyStandardMetro) .InitialVelocityFps(3100U) .ZeroDistanceYds(100.0) .ZeroImpactHeightInches(3.0); }Then:
const auto res = BuildBaseline(puut.get()) .AirPressureInHg(-1.0) .Build();Reduces 380+ duplicated lines to a handful.
HACKING.md (2)
13-14: Minor wording & comma nitThe sentence is long and misses the compound-sentence comma flagged by LanguageTool. A small tweak improves readability:
-Build system targets that are only useful for developers of this project are hidden if the `LOB_DEVELOPER_MODE` option is disabled. +Build-system targets that are only useful for developers of this project are hidden **when** the `LOB_DEVELOPER_MODE` option is disabled, and become visible once it is enabled.
21-22: Missing reference definition[12]The link label
12is used above but is not defined at the bottom of the document, triggeringmarkdownlintMD052. Add a reference such as:[12]: https://cmake.org/download/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CMakeLists.txt(3 hunks)CMakePresets.json(1 hunks)HACKING.md(1 hunks)cmake/variables.cmake(1 hunks)flake.nix(4 hunks)include/lob/lob.hpp(11 hunks)source/lob_builder.cpp(11 hunks)test/source/lob_builder_test.cpp(1 hunks)test/source/lob_cwaj_test.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CMakePresets.json
🚧 Files skipped from review as they are similar to previous changes (3)
- test/source/lob_cwaj_test.cpp
- CMakeLists.txt
- include/lob/lob.hpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
source/lob_builder.cpp (2)
source/eng_units.hpp (4)
isnan(171-173)isnan(171-171)isnan(911-913)isnan(911-911)include/lob/lob.hpp (19)
AltitudeOfBarometerFt(324-324)AltitudeOfThermometerFt(342-342)AzimuthDeg(387-387)BallisticCoefficientPsi(155-155)AirPressureInHg(313-313)BaseDiameterInch(190-190)InitialVelocityFps(259-259)LatitudeDeg(395-395)LengthInch(197-197)MachVsDragTable(230-231)MachVsDragTable(242-245)MachVsDragTable(242-243)MaximumTime(426-426)MeplatDiameterInch(183-183)OgiveRtR(218-218)RangeAngleDeg(402-402)WindHeadingDeg(365-365)ZeroDistanceYds(290-290)Reset(439-439)
🪛 LanguageTool
HACKING.md
[uncategorized] ~13-~13: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... that you are a consumer of this project and thus you have no need for these targets...
(COMMA_COMPOUND_SENTENCE)
[style] ~19-~19: You have already used this phrasing in nearby sentences. Shortening it will avoid wordiness.
Context: ...[latest CMake version][12] installed to make use of the latest Quality-of-Life additions. ...
(REP_MAKE_USE_OF)
🪛 markdownlint-cli2 (0.17.2)
HACKING.md
19-19: Reference links and images should use a label that is defined
Missing link or image reference definition: "12"
(MD052, reference-links-images)
🔇 Additional comments (3)
cmake/variables.cmake (1)
12-12: ```shell
#!/usr/bin/env bash
set -euo pipefailecho "Searching for deprecated 'lob_DEVELOPER_MODE' occurrences in the repository..."
rg -n 'lob_DEVELOPER_MODE' .</details> <details> <summary>flake.nix (2)</summary> `37-40`: **Confirm all Nix builds still pass `LOB_DEVELOPER_MODE`** The variable rename is correctly reflected here, but any remaining occurrence of the old spelling (`lob_DEVELOPER_MODE`) in the generated `CMakeUserPresets.json` or other Nix phases will now be ignored. Double-check the preset template below (`inherits = ["dev-mode", …]`) as that preset might still be setting the legacy name. --- `145-158`: **Environment variable expansion inside here-doc** `$NIX_BUILD_CORES` is expanded by the shell *before* the JSON is written, so the resulting `jobs` field holds a concrete integer — perfect. No action needed. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| const bool kIsValid = (0.0 < value); | ||
| if (!kIsValid) { | ||
| pimpl_->build.error = ErrorT::kHumidity; | ||
| } | ||
| pimpl_->relative_humidity_percent = value; |
There was a problem hiding this comment.
Relative-humidity check blocks 0 % and > 100 %
kIsValid is defined as (0.0 < value) which rejects a perfectly valid 0 % RH and does not exclude impossibly high values (> 100 %).
Use an inclusive lower bound and add an upper bound of 100 %:
- const bool kIsValid = (0.0 < value);
+ const bool kIsValid = (0.0 <= value && value <= 100.0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const bool kIsValid = (0.0 < value); | |
| if (!kIsValid) { | |
| pimpl_->build.error = ErrorT::kHumidity; | |
| } | |
| pimpl_->relative_humidity_percent = value; | |
| const bool kIsValid = (0.0 <= value && value <= 100.0); | |
| if (!kIsValid) { | |
| pimpl_->build.error = ErrorT::kHumidity; | |
| } | |
| pimpl_->relative_humidity_percent = value; |
🤖 Prompt for AI Agents
In source/lob_builder.cpp around lines 320 to 324, the relative humidity
validation incorrectly excludes 0% and does not reject values above 100%. Modify
the condition defining kIsValid to use an inclusive lower bound (value >= 0.0)
and add an upper bound check (value <= 100.0) to ensure the value is within the
valid range of 0 to 100 percent.
| stdenv = pkgs.clangStdenv; | ||
| buildInputs = oldAttrs.buildInputs ++ extraDevPackages; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Shadowing stdenv inside shellHook
inherit (pkgs) stdenv; pulls pkgs.stdenv ( = gcc by default), not the overridden clangStdenv you just assigned. That makes stdenv.isLinux / .isDarwin consult the wrong platform record.
- inherit (pkgs) stdenv;
+ inherit stdenv; # use the overridden clangStdenvCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In flake.nix around lines 102 to 103, the shellHook is inheriting stdenv from
pkgs, which is the default gcc stdenv, not the overridden clangStdenv assigned
to stdenv. To fix this, avoid inheriting stdenv from pkgs inside shellHook and
instead use the already overridden stdenv variable to ensure stdenv.isLinux and
stdenv.isDarwin refer to the correct platform record.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Other