-
Notifications
You must be signed in to change notification settings - Fork 4
Add throw_on_empty param to updatePaths
#374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #374 +/- ##
==========================================
- Coverage 83.32% 83.27% -0.05%
==========================================
Files 52 53 +1
Lines 5252 5275 +23
Branches 603 606 +3
==========================================
+ Hits 4376 4393 +17
- Misses 864 871 +7
+ Partials 12 11 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| /// @brief Update the paths of the itineraries based on the given weight function | ||
| void updatePaths(); | ||
| void updatePaths(bool const throw_on_empty = true); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 17.8 rule Note
|
|
||
| /// @brief Update the paths of the itineraries based on the given weight function | ||
| void updatePaths(); | ||
| void updatePaths(bool const throw_on_empty = true); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 13.4 rule Note
| pItinerary->id(), | ||
| pItinerary->destination()); | ||
| emptyItineraries.push_back(pItinerary->id()); | ||
| return; |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 14.4 rule Note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the path validation functionality in the road dynamics system by making exception handling for empty paths configurable. The implementation allows users to either throw exceptions on invalid paths (default behavior) or silently remove problematic itineraries with warnings.
Key Changes:
- Added a
throw_on_emptyparameter toupdatePaths()to control behavior when itineraries have no valid paths - Refactored Itinerary getters to be inlined for better performance
- Added comprehensive test coverage for both throwing and non-throwing scenarios
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dsf/mobility/RoadDynamics.hpp | Added throw_on_empty parameter to updatePaths(), implemented conditional error handling and automatic removal of invalid itineraries |
| src/dsf/mobility/Itinerary.hpp | Inlined simple getter methods (id(), destination(), path()) and added new empty() method for path validation |
| src/dsf/mobility/Itinerary.cpp | Removed function definitions that were moved inline to the header |
| src/dsf/bindings.cpp | Updated Python bindings to expose the new throw_on_empty parameter with proper default value |
| test/mobility/Test_dynamics.cpp | Added test cases for both throw_on_empty=true and throw_on_empty=false scenarios, commented out debug logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m_destinationNodes.erase(destination); | ||
| m_originNodes.erase(destination); |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple itineraries can share the same destination node. Unconditionally erasing destination from m_destinationNodes and m_originNodes when removing an itinerary will incorrectly remove these nodes even if other valid itineraries still use them as destinations.
Consider checking if any remaining itineraries use the same destination before removing it from these maps, or maintain reference counts for shared destinations.
| m_destinationNodes.erase(destination); | |
| m_originNodes.erase(destination); | |
| // Only erase destination/origin nodes if no other itinerary uses this destination | |
| bool destinationStillUsed = std::any_of( | |
| m_itineraries.begin(), m_itineraries.end(), | |
| [id, destination](const auto& pair) { | |
| return pair.first != id && pair.second->destination() == destination; | |
| }); | |
| if (!destinationStillUsed) { | |
| m_destinationNodes.erase(destination); | |
| m_originNodes.erase(destination); | |
| } |
Based on @filippodll request