Skip to content

Conversation

alexbeattie42
Copy link
Contributor

@alexbeattie42 alexbeattie42 commented Jun 17, 2025

Fixes issue #4095

Brief summary of changes

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

  • no need to update because...
  • updated.

This change is Reviewable

@alexbeattie42
Copy link
Contributor Author

@nickbianco is this a suitable fix for the compiler warning?

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

@alexbeattie42 let's try overriding Object's virtual methods to see if that resolves the compiler warnings.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alexbeattie42)


OpenSim/Simulation/Control/ControlLinearNode.h line 99 at r1 (raw file):

    ControlLinearNode& operator=(const ControlLinearNode &aControl);
    bool operator==(const ControlLinearNode &aControl) const;
    bool operator<(const ControlLinearNode &aControl) const;

I think we should only need to add override calls to Objects virtual methods. We should be able to leave the copy assignment operator for ControlLinearNode as-is since it not a virtual method of Object.

Suggestion:

    ControlLinearNode& operator=(const ControlLinearNode &aControl);
    bool operator==(const ControlLinearNode &aControl) const override;
    bool operator<(const ControlLinearNode &aControl) const override;

@alexbeattie42 alexbeattie42 force-pushed the alex/operator-overshadow branch from 1fd7b72 to 2b7796e Compare July 24, 2025 11:12
@alexbeattie42
Copy link
Contributor Author

OpenSim/Simulation/Control/ControlLinearNode.h line 99 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I think we should only need to add override calls to Objects virtual methods. We should be able to leave the copy assignment operator for ControlLinearNode as-is since it not a virtual method of Object.

If I make this change clang complains that:
Only virtual member functions can be marked 'override'

Copy link
Contributor Author

@alexbeattie42 alexbeattie42 left a comment

Choose a reason for hiding this comment

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

I tried the suggested approach and clang is not happy with it sadly.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nickbianco)

@nickbianco
Copy link
Member

Ah, that is because the signatures are different. To truly satisfy Object's interface, you would need to override the operators using Object types instead of ControlLinearNode:

bool operator==(const Object& other) const override;
bool operator<(const Object& other) const override;

This is an odd quirk of polymorphism that I hadn't considered before. I suppose you could do that, but then you would need to dynamic_cast to ControlLinearNode internally which seems hacky. But I'm not sure having using Object::operator== is ideal either.

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.

2 participants