Skip to content

Conversation

@stuqdog
Copy link
Member

@stuqdog stuqdog commented Feb 21, 2024

No description provided.

@stuqdog stuqdog requested a review from a team as a code owner February 21, 2024 18:19
@stuqdog stuqdog requested review from benjirewis and njooma and removed request for a team and benjirewis February 21, 2024 18:19
const Name& component_name,
const Name& slam_name,
const std::shared_ptr<motion_configuration>& motion_configuration,
const std::vector<GeometryConfig>& obstacles,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my knowledge of cpp is very limited so I want to confirm that vector means list.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! That's correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly "list" - more like "dynamic array". Both std::vector and std::list are dynamically sized and bidirectionally iterable. The key difference is that std::vector is guaranteed contiguous, but insertions/deletions can cost reallocation / element-by-element copies, whereas std::list is non-contiguous, but insertions/deletions anywhere are cheap since contiguity isn't in play.

*request.mutable_component_name() = component_name.to_proto();
*request.mutable_slam_service_name() = slam_name.to_proto();

for (const auto& obstacle : obstacles) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[q]: does anything special need to happen if the user chooses to not specify any obstacles e.g. use nil?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So a user couldn't pass nullptr or anything like that because the arg is not a pointer. If they wanted to not specify any obstacles they would want to pass an empty vec (trivial to do, just pass {} as the arg). Nothing special is required in that case.

Copy link
Member

@nfranczak nfranczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

Copy link
Member

@acmorrow acmorrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

const Name& component_name,
const Name& slam_name,
const std::shared_ptr<motion_configuration>& motion_configuration,
const std::vector<GeometryConfig>& obstacles,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly "list" - more like "dynamic array". Both std::vector and std::list are dynamically sized and bidirectionally iterable. The key difference is that std::vector is guaranteed contiguous, but insertions/deletions can cost reallocation / element-by-element copies, whereas std::list is non-contiguous, but insertions/deletions anywhere are cheap since contiguity isn't in play.

@stuqdog stuqdog merged commit 8891d79 into viamrobotics:main Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants