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

Remove unnecessary symbol export in header files for API management #123

Open
3 tasks done
soblin opened this issue Dec 12, 2024 · 0 comments
Open
3 tasks done

Remove unnecessary symbol export in header files for API management #123

soblin opened this issue Dec 12, 2024 · 0 comments

Comments

@soblin
Copy link

soblin commented Dec 12, 2024

Checklist

  • I've read the contribution guidelines.
  • I've searched other issues and no duplicate issues were found.
  • I've agreed with the maintainers that I can plan this task.

Description

Many packages ported from autoware.universe follows the possibly bad practice of inappropriate usage of using in header files. Here is an suspicious example:

using MapProjectorInfo = autoware_map_msgs::msg::MapProjectorInfo;
using GeoPoint = geographic_msgs::msg::GeoPoint;
using LocalPoint = geometry_msgs::msg::Point;

The appropriate usage of using directive in header file is to export the target symbol name as the library module's interface name which all the dependent modules should use when using the library.

Especially this practice is useful if the symbol name originates from a external library,

using Symbol = Foo_v1;

since it makes it easy for us to switch the external library just by renaming the symbol without forcing the library callee side to rename Foo to NewFoo, because if they use Symbol, not Foo in their codebase appropriately, following change remains transparent for them -- which is exactly the role of what's known as API compatibility.

using Symbol = NewFoo;

Below code snippet should remain valid before/after the renaming.

// callee side
void do_stuff(const Symbol & symbol) {
  symbol.call_foo();
}

The issue of current usage

using MapProjectorInfo = autoware_map_msgs::msg::MapProjectorInfo; // I guess it does not make any sense

using GeoPoint = geographic_msgs::msg::GeoPoint; // this seems OK to me, if we ensure that we may redefine `GeoPoint` as a new class in the future and also we promise that all the public API of geographic_msgs::msg::GeoPoint is inherited 
using LocalPoint = geometry_msgs::msg::Point; // seems OK well

is that MapProjectorInfo symbol name is a common identifier in our autoware project and it's very likely that other developers already use fully-qualified autoware_map_msgs::msg::MapProjectorInfo in the same way as autoware::geography_utils::MapProjectorInfo accidentally. Thus it does not serve as a API.

NOTE:

The user should also pass LocalPoint{x=, y=, z=} instead of geometry_msgs::msg::Point{x=, y=, z=}.

Purpose

For removing unnecessary usings in header files.

Possible approaches

We need to check which symbol is our API for customization point and which is not.

Definition of done

Write a guideline in documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To Triage
Development

No branches or pull requests

1 participant