ros2 / message_filters

BSD 3-Clause "New" or "Revised" License
76 stars 66 forks source link

Adding cpplint #125

Closed CursedRock17 closed 6 months ago

CursedRock17 commented 6 months ago

This pull request is part of a group of pull request which break apart #120, this particular pull request adds cpplint to message_filters.

CursedRock17 commented 6 months ago

So I've fixed the issues with cpplint though there are concerns with the explicit constructors.

  1. When it comes to MessageEvent, there was a call to make its constructor explicit in ROS 2, which is fine, but it means callbacks must follow the type of const ConstMessagePtr &. Thus, this line was changed to use the correct callbacks, creating errors otherwise. These changes now result in an API break, so is there a way to silence the explication warnings for this, should I create explicit constructors for each message type, or should we move on regardless?
  2. There are also issues with the explicit keyword within sync_policies, in which the Policy must be constructed and then passed to the Synchronizer like here. Not a big deal work wise, but technically this will also cause an API break, which isn't a correct practice to follow, so should we also silence these errors instead of creating an explicit constructor within the policies?
clalancette commented 6 months ago

So I've fixed the issues with cpplint though there are concerns with the explicit constructors.

So technically, marking any of these constructors explicit is an API break; things that compiled before may no longer compile.

While that isn't necessarily a bad thing, we should probably decouple it from this PR so we can get this in. In that case, what I'm going to suggest is that everywhere cpplint complains about explicit constructors, we add a // NOLINT(explicit) to the line. Once we get this PR in, we can consider whether we actually want to do that API break separately. Does that sound reasonable?