Add common linters to tf2 #258

ahcorde · 2020-04-28T13:30:10Z

This PR adds common linters. It builds on top of this other PR #257.

uncrustify
cppcheck
cpplint
copyright headers

ahcorde · 2020-04-28T13:35:05Z

I still have some issues with some copyright headers located in tf2/LinearMath. This copyright header is no defined in the templates. Any thoughts about how to include this particular copyrights?

clalancette · 2020-04-28T13:40:39Z

I still have some issues with some copyright headers located in tf2/LinearMath. This copyright header is no defined in the templates. Any thoughts about how to include this particular copyrights?

Ug. That looks like some kind of MIT derivative, or something. We'll have to do some research on it; for now, I'll suggest that we just disable copyright checking.

clalancette

Overall, this looks pretty good. A lot of little nits inline. There are also some bigger things listed below. But since I don't want this PR to get much bigger, please open issues for these so we can follow-up later:

Rename all .h -> .hpp files. This will have to also have a deprecation warning, and should be done post-Foxy.
Go through the comments (particularly API comments) and fix the ones that say todo.
Go through the comments and use a common Doxygen style (some of the comments use \ and some use @).

tf2/include/tf2/LinearMath/Matrix3x3.h

tf2/include/tf2/LinearMath/QuadWord.h

tf2/include/tf2/LinearMath/Quaternion.h

tf2/include/tf2/time.h

tf2/include/tf2/time_cache.h

clalancette · 2020-04-28T14:16:03Z

tf2/include/tf2/transform_storage.h

  {
    *this = rhs;
  }

  TF2_PUBLIC
-  TransformStorage& operator=(const TransformStorage& rhs)
+  TransformStorage & operator=(const TransformStorage & rhs)
  {
 #if 01


This looks like a bug. If you don't want to deal with it in this PR, please open an issue to follow-up on it.

tf2/src/buffer_core.cpp

ahcorde · 2020-04-28T15:14:28Z

Thanks for the review! it's quite long the PR.

The issue was created #259

ahcorde · 2020-04-28T15:56:07Z

For now I will not remove because it's part of the public API. Maybe in the post-foxy release review.

clalancette

Two more minor things to fix, then I think we're good to go.

One question: in #259, what do you mean by "remove time.h"? I think we added that relatively recently so that people could just get the time conversions, but maybe I'm misremembering. Can you elaborate more on why?

clalancette · 2020-04-29T16:51:32Z

tf2/include/tf2/time.h

+#ifndef TF2__TIME_H_
+#define TF2__TIME_H_
+
+#include <stdio.h>


This comment about removing stdio.h is still open.

tf2/include/tf2/time.h

ahcorde · 2020-04-29T17:07:30Z

I understood the comment wrong. I read that the "file" was not used, not just the #include <stdio.h>. Sorry about that, I updated the issue

clalancette

Looks good with green CI.

ahcorde · 2020-04-29T19:27:20Z

Linux
Linux-aarch64
macOS
Windows

tfoote · 2020-04-29T22:25:25Z

I'd really like to not apply the linters to LinearMath files. It's a fork of Bullets LinearMath implementation and we periodically sync between them. Can we suppress the linting in that subdirectory?

ahcorde · 2020-04-30T13:42:26Z

For now we can only suppress the linting in uncrustify, but with this PR ament/ament_lint#234 we can suppress them also in cppcheck and cpplint

tfoote

Thanks this looks much better with excluding the LinearMath content.

clalancette · 2020-05-01T18:30:05Z

So, while I think we're good to go here, I would advocate for not merging this PR until after we branch off for foxy. The risk of regression is fairly low, but it's not zero and I think we should let foxy calm down a bit. @ahcorde @tfoote what do you think?

dirk-thomas · 2020-05-01T20:11:59Z

Since Foxy will be supported for 3+ years I would highly suggest to not have such a big diff between the Foxy and G-turtle branch.

clalancette · 2020-05-01T20:16:48Z

Since Foxy will be supported for 3+ years I would highly suggest to not have such a big diff between the Foxy and G-turtle branch.

Why?

dirk-thomas · 2020-05-01T20:32:39Z

Because of the extra effort to backport fixes.

ahcorde · 2020-05-04T07:04:04Z

This package is one of the candidates for Quality Level 1 or 2. This means that linters is a must. what's the best way to proceed?

clalancette · 2020-05-04T14:23:36Z

My opinion hasn't changed; if we keep shoveling stuff into Foxy, then it will never get done.

That being said, if others think we should proceed, then I won't stand in the way.

tfoote · 2020-05-15T18:36:42Z

Clearly it's a risk, but I think continuity with G-Turtle and forward will be more valuable than backwards compatibility with Noetic so the maintenance hopefully will be reduced. We're already pretty diverged from the ROS 1 branch so patches won't just apply across.

Clearly any big change is a risk but this has reasonable coverage so I think it's relatively low especially with it theoretically being just reformatting.

@ros-pull-request-builder retest this please

ahcorde · 2020-05-18T07:52:48Z

@tfoote the pull-request-builder failed this other PR should resolve the issue #267

ahcorde · 2020-05-19T17:40:53Z

@ros-pull-request-builder retest this please

ahcorde · 2020-05-29T17:36:14Z

Testing changes on ament_lint ament/ament_lint#238. Running CI up-to tf2

Linux
Linux-aarch64
macOS
Windows

clalancette · 2020-06-11T19:26:21Z

I've now rebased and force-pushed this on the latest. It still needs the changes from ament/ament_lint#238 to go in first, though.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> Signed-off-by: ahcorde <ahcorde@gmail.com>

clalancette · 2020-08-06T15:01:46Z

I've rebased this onto the latest, and also replaced the actual linting checks with a TODO.

I'd like to go ahead and merge this, since this is going to affect any incoming PRs that touch the tf2 directory. We won't have the actual checks in place until we resolve ament/ament_lint#238 , but it should be a much smaller patch once we do that.

With all of that said, then, here is another CI up to tf2. If this succeeds, I'll merge:

Linux
Linux-aarch64
macOS
Windows

Each of the common linters were individually `find_package`-d and invoked in the CMakeLists.txt file. This is because there is no simple way to pass file exclusion information through `ament_lint_auto`. File exclusion is necessary because we do not want to run linters on the include/tf2/LinearMath/ headers, since these are external and periodically synced with upstream. See #258 (comment) for more information. Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

As discussed in #467: - Enables a subset of relevant linter tests from `ament_lint_common` in `tf2`. - Excludes LinearMath headers (include/tf2/LinearMath) from being linted. - Fixes line-length formatting issues in a few source files, so that there are no lint failures in the current state. Note that individual linters had to be invoked rather than use `ament_lint_auto` because the LinearMath headers should not have linters run on them - see #258 (comment) for more information - and because `ament_lint_auto` does not provide a general way of providing file exclusion lists. Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

ahcorde added the enhancement New feature or request label Apr 28, 2020

ahcorde requested a review from tfoote April 28, 2020 13:30

ahcorde self-assigned this Apr 28, 2020

clalancette reviewed Apr 28, 2020

View reviewed changes

ahcorde mentioned this pull request Apr 28, 2020

Improvements in tf2 (related with documentation and style) #259

Open

4 tasks

ahcorde changed the base branch from ahcorde/doc/doxifiles to ros2 April 29, 2020 15:55

ahcorde requested a review from clalancette April 29, 2020 15:58

clalancette reviewed Apr 29, 2020

View reviewed changes

clalancette approved these changes Apr 29, 2020

View reviewed changes

ahcorde mentioned this pull request Apr 30, 2020

Updated cpplint and cppcheck to exclude directories and files ament/ament_lint#234

Merged

tfoote approved these changes May 1, 2020

View reviewed changes

ahcorde mentioned this pull request May 18, 2020

Fixed in Exception constructor in tf2_ros #267

Merged

ahcorde mentioned this pull request May 29, 2020

Exclude files or directories in cpplint.py ament/ament_lint#238

Closed

clalancette force-pushed the ahcorde/docs/improve branch from bec7635 to dc9126d Compare June 11, 2020 19:25

Add common linters to tf2.

dd033ec

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> Signed-off-by: ahcorde <ahcorde@gmail.com>

clalancette force-pushed the ahcorde/docs/improve branch from dc9126d to dd033ec Compare August 6, 2020 14:59

clalancette merged commit 7b0f16f into ros2 Aug 6, 2020

clalancette deleted the ahcorde/docs/improve branch August 6, 2020 16:56

aprotyas mentioned this pull request Oct 17, 2021

tf2: Enable common linter tests #469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add common linters to tf2 #258

Add common linters to tf2 #258

ahcorde commented Apr 28, 2020

ahcorde commented Apr 28, 2020

clalancette commented Apr 28, 2020

clalancette left a comment

clalancette Apr 28, 2020

ahcorde commented Apr 28, 2020

ahcorde commented Apr 28, 2020

clalancette left a comment

clalancette Apr 29, 2020

ahcorde commented Apr 29, 2020

clalancette left a comment

ahcorde commented Apr 29, 2020

tfoote commented Apr 29, 2020

ahcorde commented Apr 30, 2020

tfoote left a comment

clalancette commented May 1, 2020

dirk-thomas commented May 1, 2020

clalancette commented May 1, 2020

dirk-thomas commented May 1, 2020

ahcorde commented May 4, 2020

clalancette commented May 4, 2020

tfoote commented May 15, 2020

ahcorde commented May 18, 2020

ahcorde commented May 19, 2020

ahcorde commented May 29, 2020

clalancette commented Jun 11, 2020

clalancette commented Aug 6, 2020

Add common linters to tf2 #258

Add common linters to tf2 #258

Conversation

ahcorde commented Apr 28, 2020

ahcorde commented Apr 28, 2020

clalancette commented Apr 28, 2020

clalancette left a comment

Choose a reason for hiding this comment

clalancette Apr 28, 2020

Choose a reason for hiding this comment

ahcorde commented Apr 28, 2020

ahcorde commented Apr 28, 2020

clalancette left a comment

Choose a reason for hiding this comment

clalancette Apr 29, 2020

Choose a reason for hiding this comment

ahcorde commented Apr 29, 2020

clalancette left a comment

Choose a reason for hiding this comment

ahcorde commented Apr 29, 2020

tfoote commented Apr 29, 2020

ahcorde commented Apr 30, 2020

tfoote left a comment

Choose a reason for hiding this comment

clalancette commented May 1, 2020

dirk-thomas commented May 1, 2020

clalancette commented May 1, 2020

dirk-thomas commented May 1, 2020

ahcorde commented May 4, 2020

clalancette commented May 4, 2020

tfoote commented May 15, 2020

ahcorde commented May 18, 2020

ahcorde commented May 19, 2020

ahcorde commented May 29, 2020

clalancette commented Jun 11, 2020

clalancette commented Aug 6, 2020