Skip to content
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

[multibody] Remove dynamic_cast from private inline functions #22208

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Nov 17, 2024

The dynamic_cast operation does not meet our styleguide requirements for being inline. We either need to get rid of the cast (by simplifying the function), or move its definition to the cc file.

This change is tangentially inspired by #22204.

(Technically the GetParentPlant is a public function so the commit subject isn't quite right, but anyway it seems reasonable to change it here, too, since its fix shares a mechanism within this cohort.)


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: low status: single reviewer ok https://drake.mit.edu/reviewable.html release notes: none This pull request should not be mentioned in the release notes labels Nov 17, 2024
@jwnimmer-tri
Copy link
Collaborator Author

+@amcastro-tri for a quick feature reviewer sanity-check from the MbT developer team, please.

+@rpoyner-tri for platform review per schedule (tomorrow), please.

Copy link
Contributor

@amcastro-tri amcastro-tri left a comment

Choose a reason for hiding this comment

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

This looks good Jeremy, I just had one question.

Reviewed 23 of 23 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),amcastro-tri


multibody/plant/force_density_field.h line 18 at r1 (raw file):

template <typename T>
class MultibodyPlant;

I forget, do we need to guard this against being processed by Doxygen?


multibody/plant/force_density_field.h line 83 at r1 (raw file):

         invokes this function; force_density_field.h cannot do that for you.
   @pre tree_system != nullptr. */
  template <typename MultibodyPlantDeferred = MultibodyPlant<T>>

pardon my ignorance, do we still need this template trick?

@jwnimmer-tri jwnimmer-tri force-pushed the mbt-private-dynamic-cast branch from 539fdb0 to 2a56042 Compare November 18, 2024 21:32
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),amcastro-tri


multibody/plant/force_density_field.h line 18 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

I forget, do we need to guard this against being processed by Doxygen?

Not every forward declaration confuses Doxygen, but some do, and I don't know what detail(s) of the forward declaration are relevant to misleading Doxygen's parsing.

When I am writing new code I do always guard forward declarations with with an ifndef, but I'm not going to change this pre-existing code unless we find a specific cause for concern.


multibody/plant/force_density_field.h line 83 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

pardon my ignorance, do we still need this template trick?

We still need it but it doesn't matter what type it mentions. Simplified to use void, instead.

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 15 of 23 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee amcastro-tri

@jwnimmer-tri jwnimmer-tri force-pushed the mbt-private-dynamic-cast branch from 2a56042 to aede63b Compare November 22, 2024 17:41
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Dismissed @amcastro-tri from 2 discussions.
Reviewable status: LGTM missing from assignee amcastro-tri

@jwnimmer-tri
Copy link
Collaborator Author

@amcastro-tri we're missing the LGTM from you, please.

@jwnimmer-tri
Copy link
Collaborator Author

I'll deem the "sanity-check from the MbT developer team" sufficiently completed.

@jwnimmer-tri jwnimmer-tri merged commit 1c1bc16 into RobotLocomotion:master Nov 26, 2024
10 checks passed
@jwnimmer-tri jwnimmer-tri deleted the mbt-private-dynamic-cast branch November 26, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: none This pull request should not be mentioned in the release notes status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants