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

Unnecessary equal operators #1114

Closed
TauTheLepton opened this issue Oct 24, 2023 · 5 comments
Closed

Unnecessary equal operators #1114

TauTheLepton opened this issue Oct 24, 2023 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@TauTheLepton
Copy link
Contributor

Describe the bug
The functions below generate compilation errors.

bool operator==(const geometry_msgs::msg::Point & v0, const geometry_msgs::msg::Point & v1);
bool operator==(const geometry_msgs::msg::Vector3 & v0, const geometry_msgs::msg::Vector3 & v1);

Both equal operators are ambiguous. ROS2 message types have automatically generated equal operator member function. Any usage of the aforementioned functions results in a compilation error.
It is unnecessary to keep functions that cannot be used, so I suggest removing these functions.

Context:
I am adding unit tests for the geometry package and wanted to add tests for these functions as well, but as it turns out they cannot be used.

To Reproduce
Steps to reproduce the behavior:

  1. Edit any source code file or add a new one
  2. Include the linear_algebra.hpp header file
  3. Write any code that uses the equal operator
  4. Try to compile
  5. See error

Expected behavior
Using Point and Vector3 equal operator should result in a correct evaluation and not in a compilation error.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: [e.g. iOS]: Ubuntu
  • Browser [e.g. chrome, safari]: Firefox
  • Version [e.g. 22]: 22
  • ROS 2 version: Humble
  • DDS: CycloneDDS
@hakuturu583
Copy link
Collaborator

hakuturu583 commented Oct 25, 2023

Both equal operators are ambiguous. ROS2 message types have automatically generated equal operator member function. Any usage of the aforementioned functions results in a compilation error.
It is unnecessary to keep functions that cannot be used, so I suggest removing these functions.

Thank you!
Is the ROS2 message types have automatically generated equal operator consider machine epsilon?

I build common_interfaces package and checking automatically generated equal operator for Float64 type value, it seems it is not consider machine epsilon.

  bool operator==(const Float64_ & other) const
  {
    if (this->data != other.data) {
      return false;
    }
    return true;
  }

@hakuturu583
Copy link
Collaborator

If the operator does not consider machine epsilon, I think the automatically generated operator should not be used and compilation error does not raised in my environment and CI/CD environment.

@hakuturu583 hakuturu583 self-assigned this Oct 25, 2023
@TauTheLepton
Copy link
Contributor Author

The default operator does not consider epsilon as you said, but it can be confusing to have to use the operator explicitly.
The error is raised only when the operator is used in such a way:

vector0 == vector1;

This is the most common way to use the operator I think.

I have created an example test case showcasing the problem here.

When compiling the code from this branch the error can be observed.

I found it confusing to have to use one operator explicitly as I have not found any note about this. However if you consider this to be a correct behavior then please close this issue.

@hakuturu583
Copy link
Collaborator

I see, I encounter error.
It seems, this operator does not used in scenario_simulator_v2 so please remove the unnecessary equal operator.

@hakuturu583 hakuturu583 added the bug Something isn't working label Oct 26, 2023
@hakuturu583
Copy link
Collaborator

#1139 was merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants