Replace return of `std::unique_ptr<T>&` by Grufoony · Pull Request #436 · physycom/DynamicalSystemFramework

Grufoony · 2026-04-13T09:30:01Z

No description provided.

codecov · 2026-04-13T09:33:00Z

Codecov Report

❌ Patch coverage is 81.65680% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.35%. Comparing base (b49bf11) to head (a53f5a3).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/dsf/mobility/FirstOrderDynamics.cpp	58.69%	19 Missing ⚠️
src/dsf/mobility/RoadNetwork.cpp	73.91%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
+ Coverage   87.33%   87.35%   +0.02%     
==========================================
  Files          52       52              
  Lines        6520     6510      -10     
  Branches      731      730       -1     
==========================================
- Hits         5694     5687       -7     
+ Misses        804      801       -3     
  Partials       22       22

Flag	Coverage Δ
unittests	`87.35% <81.65%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-advanced-security

Cppcheck (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Copilot

Pull request overview

This PR updates the graph/network APIs to stop exposing std::unique_ptr<T>& (and related pointer-based call sites), switching callers to work with references/pointers to the underlying node/edge objects instead. This cascades through mobility algorithms, tests, and Python bindings, and bumps the project version.

Changes:

Refactor Network/RoadNetwork accessors and path-weight callables to use T& / T const& instead of std::unique_ptr<T> const&.
Update mobility algorithms, tests, and bindings to use . access and reference-based lambdas.
Bump library/version metadata to 5.3.4.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
test/mobility/Test_graph.cpp	Updates tests for new edge/node access patterns; adjusts path-weight lambdas.
test/mobility/Test_dynamics.cpp	Updates dynamics tests to use new reference-based accessors.
src/dsf/mobility/RoadNetwork.hpp	Updates `street()` return type and template constraints for weight functions.
src/dsf/mobility/RoadNetwork.cpp	Adapts road import/mutation logic to new accessor semantics and direct unique_ptr replacement where needed.
src/dsf/mobility/FirstOrderDynamics.hpp	Changes speed/weight function signatures and internal evolution helpers to work with `Street const&` and raw pointers.
src/dsf/mobility/FirstOrderDynamics.cpp	Propagates reference/pointer API changes throughout simulation evolution and path weighting.
src/dsf/bindings.cpp	Updates pybind lambdas to accept `Street const&` and use `.` access.
src/dsf/base/Network.hpp	Replaces unique_ptr-returning accessors with reference-returning accessors; updates centrality APIs accordingly.
src/dsf/dsf.hpp	Version patch bump to 5.3.4.
CITATION.cff	Updates release/version metadata to 5.3.4.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/dsf/base/Network.hpp

Copilot · 2026-04-13T09:34:08Z

test/mobility/Test_graph.cpp

+    REQUIRE(graph.edge(static_cast<Id>(0)).betweennessCentrality().has_value());
+    CHECK_EQ(graph.edge(static_cast<Id>(0)).betweennessCentrality(),
             doctest::Approx(3.0));


Edge::betweennessCentrality() returns std::optional<double>; these CHECK_EQ lines compare the optional itself to doctest::Approx, which won’t compile / won’t test the underlying value. Dereference or use .value() (similar to how node BC is asserted above).

test/mobility/Test_graph.cpp

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-13T11:36:26Z

test/mobility/Test_graph.cpp

+    REQUIRE(graph.edge(static_cast<Id>(1)).betweennessCentrality().has_value());
+    CHECK_EQ(graph.edge(static_cast<Id>(1)).betweennessCentrality(),
             doctest::Approx(4.0));


Same issue as above: after REQUIRE(...has_value()), the test should compare the contained double, not the std::optional<double> itself. Dereference or call .value() before comparing with doctest::Approx.

Copilot · 2026-04-13T11:36:26Z

test/mobility/Test_graph.cpp

+    REQUIRE(graph.edge(static_cast<Id>(2)).betweennessCentrality().has_value());
+    CHECK_EQ(graph.edge(static_cast<Id>(2)).betweennessCentrality(),
             doctest::Approx(3.0));


Same issue as above: CHECK_EQ(graph.edge(...).betweennessCentrality(), doctest::Approx(...)) is comparing an std::optional<double> to an Approx. Dereference or use .value() after the REQUIRE.

Copilot · 2026-04-13T11:36:26Z

test/mobility/Test_graph.cpp

+    REQUIRE(graph.edge(static_cast<Id>(1)).betweennessCentrality().has_value());
+    CHECK_EQ(graph.edge(static_cast<Id>(1)).betweennessCentrality(),
             doctest::Approx(1.0));


Same optional-vs-doctest::Approx comparison issue here: after REQUIRE(...has_value()), dereference or use .value() when comparing betweenness centrality to Approx.

Copilot · 2026-04-13T11:36:27Z

test/mobility/Test_graph.cpp

+    REQUIRE(graph.edge(static_cast<Id>(2)).betweennessCentrality().has_value());
+    CHECK_EQ(graph.edge(static_cast<Id>(2)).betweennessCentrality(),
             doctest::Approx(2.0));


Same optional-vs-doctest::Approx comparison issue here: betweennessCentrality() returns std::optional<double>, so this should compare the contained double value (dereference or .value()).

Copilot · 2026-04-13T11:36:27Z

test/mobility/Test_graph.cpp

+    REQUIRE(graph.edge(static_cast<Id>(3)).betweennessCentrality().has_value());
+    CHECK_EQ(graph.edge(static_cast<Id>(3)).betweennessCentrality(),
             doctest::Approx(1.0));


Same optional-vs-doctest::Approx comparison issue here: compare *...betweennessCentrality() (or .value()) instead of the std::optional<double> object.

Copilot · 2026-04-13T11:36:27Z

src/dsf/mobility/RoadNetwork.cpp

+    auto const it = std::find_if(
+        m_edges.cbegin(), m_edges.cend(), [source, destination](auto const& pair) {
+          return pair.second->source() == source && pair.second->target() == destination;
+        });
+    if (it == m_edges.cend()) {
      return nullptr;
    }
+    return it->second.get();


RoadNetwork::street() now performs a linear scan over all edges to find a (source,destination) match. Since Network::edge(source,target) already implements the same scan+throw, consider delegating to it (try/catch + return pointer) to avoid duplicated lookup logic and reduce the chance of the two implementations drifting. If lookup by (source,target) is performance-critical, consider maintaining an index map instead of scanning m_edges.

Suggested change

auto const it = std::find_if(

m_edges.cbegin(), m_edges.cend(), [source, destination](auto const& pair) {

return pair.second->source() == source && pair.second->target() == destination;

});

if (it == m_edges.cend()) {

return nullptr;

}

return it->second.get();

try {

return &edge(source, destination);

} catch (std::out_of_range const&) {

return nullptr;

}

Grufoony added 2 commits April 13, 2026 11:21

Replace return of std::unique_ptr<T>&

807aec6

Bump version

d1b7f7b

Copilot AI review requested due to automatic review settings April 13, 2026 09:30

Copilot started reviewing on behalf of Grufoony April 13, 2026 09:31 View session

github-advanced-security AI found potential problems Apr 13, 2026

View reviewed changes

Copilot AI reviewed Apr 13, 2026

View reviewed changes

Grufoony force-pushed the pointerstoreferences branch 3 times, most recently from 7bd8841 to 449a6f2 Compare April 13, 2026 11:28

Grufoony requested a review from Copilot April 13, 2026 11:29

Copilot started reviewing on behalf of Grufoony April 13, 2026 11:32 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

Grufoony force-pushed the pointerstoreferences branch from 449a6f2 to fa13d1d Compare April 13, 2026 11:59

Fix

a53f5a3

Grufoony force-pushed the pointerstoreferences branch from fa13d1d to a53f5a3 Compare April 13, 2026 12:17

Grufoony merged commit 8e0f47d into main Apr 13, 2026
48 of 49 checks passed

Grufoony deleted the pointerstoreferences branch April 13, 2026 12:58

Conversation

Grufoony commented Apr 13, 2026

Uh oh!

codecov bot commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-advanced-security AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Apr 13, 2026 •

edited

Loading