C++11 headers such as <future> are "unapproved" in upstream cpplint.py. Support for these should be monkey-patched in.

The following code is does not pass cpplint::whitespace/empty_loop_body check:

do
{
...
}
while (cond);

Error:

Empty loop bodies should use {} or continue  [whitespace/empty_loop_body] [5]

It looks like roslint's CPP module uses the Google style guide, which is slightly different from ROS's long-standing one (http://wiki.ros.org/CppStyleGuide). Notable items below.

ROS style guide specifies putting opening brace on it's own line, current rules throw:

{ should almost always be at the end of the previous line  [whitespace/braces] [4]
An else should appear on the same line as the preceding }  [whitespace/newline] [4]

Examples, and most of ROS core code put "public:" and similar labels at the beginning of the line. Style guide linked above also shows it this way. Causes the following to be thrown:

Labels should always be indented at least one space.  If this is a member-initializer list in a constructor or the base class list in a class definition, the colon should be on the following line.  [whitespace/labels] [4]

ROS Services use non-const references, causes:

Is this a non-const reference? If so, make const or use a pointer.  [runtime/references] [2]

This is a follow-up from http://answers.ros.org/question/246677

According to the ROS C++ style guide, function names should be camelCased (lower-case first letter), not CamelCased (capital-case first letter). However, roslint doesn't seem to complain about CamelCased function names. Actually, it doesn't complain even if a function name is under_scored 🤔

Here's a proof of concept:

void my_silly_test() {
    return;
}

And here's the output of catkin build --no-deps <package> --make-args roslint_<package>:

when starting a new scope, { should be on a line by itself  [whitespace/braces]

It correctly complains about the scope delimiter, but not the function name.

On the wiki. For example, an actual astyle config/invocation which brings C++ whitespace into compliance, and something similar for Python.

This speaks for itself:

$ sudo apt-get install pylint
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following extra packages will be installed:
  blt libdrm-intel1 libdrm-nouveau2 libdrm-radeon1 libfontenc1 libgl1-mesa-dri libgl1-mesa-glx libglapi-mesa libllvm3.2
  libpciaccess0 libtxc-dxtn-s2tc0 libutempter0 libx11-xcb1 libxcb-dri2-0 libxcb-glx0 libxcb-shape0 libxss1 libxtst6 libxv1
  libxxf86dga1 libxxf86vm1 python-egenix-mxdatetime python-egenix-mxtools python-logilab-astng python-logilab-common python-tk
  tcl8.5 tcl8.5-lib tk8.5 tk8.5-lib x11-utils xbitmaps xterm
Suggested packages:
  blt-demo libglide3 python-egenix-mxdatetime-dbg python-egenix-mxdatetime-doc python-egenix-mxtools-dbg
  python-egenix-mxtools-doc pyro python-unittest2 tix python-tk-dbg tcl-tclreadline mesa-utils xfonts-cyrillic
Recommended packages:
  libtxc-dxtn0
The following NEW packages will be installed:
  blt libdrm-intel1 libdrm-nouveau2 libdrm-radeon1 libfontenc1 libgl1-mesa-dri libgl1-mesa-glx libglapi-mesa libllvm3.2
  libpciaccess0 libtxc-dxtn-s2tc0 libutempter0 libx11-xcb1 libxcb-dri2-0 libxcb-glx0 libxcb-shape0 libxss1 libxtst6 libxv1
  libxxf86dga1 libxxf86vm1 pylint python-egenix-mxdatetime python-egenix-mxtools python-logilab-astng python-logilab-common
  python-tk tcl8.5 tcl8.5-lib tk8.5 tk8.5-lib x11-utils xbitmaps xterm
0 upgraded, 34 newly installed, 0 to remove and 150 not upgraded.
Need to get 17.1 MB of archives.
After this operation, 59.7 MB of additional disk space will be used.
Do you want to continue [Y/n]?

There's no way a linter should require 60MB of stuff. The issue is the dependency on python-logilab-astng, which brings in all kinds of xwindows junk.

Given #2 as well, I think we should swap out pylint for https://pypi.python.org/pypi/pep8

In the following file:

/*********************************************************************
 *  Copyright (c) XXXXX, Inc.
 *********************************************************************/
void *testFunction(void * /*unused_param*/)
{
}

Cpplint returns:

test.cpp:4:  Using C-style cast.  Use reinterpret_cast<void *>(...) instead  [readability/casting] [4]

In cpplint.py here there is already a case to cover this. But the problem is that this condition only gets checked if the line ends (among other things) with the character {, as you can see here.

So that case would be correctly caught if we use

/*********************************************************************
 *  Copyright (c) XXXXX, Inc.
 *********************************************************************/
void *testFunction(void * /*unused_param*/) {
}

but then of course we get an error about the position of the braces.

test.cpp:4:  when starting a new scope, { should be on a line by itself  [whitespace/braces] [4]

cpplint currently doesn't like do while loops and erroneously warns about them. The problem seems to be due to the change in brace styles: https://github.com/ros/roslint/blob/master/src/roslint/cpplint.py#L3130

Given:

// Copyright 2015 Alex Henning

void dowhile()
{
  do
  {
    // Do some stuff
  }
  while (true);
}

Result:

dowhile.cpp:9:  Empty loop bodies should use {} or continue  [whitespace/empty_loop_body] [5]
Done processing dowhile.cpp
Total errors found: 1

Right now, the roslint_python and roslint_cpp macros support being passed lists of source files, or nothing, where the "nothing" behaviour is to glob and lint everything in the package. It would be great to be able to specify a path to lint everything below, eg:

roslint_cpp(DIRECTORY include)

Part of this could be supporting an optional corresponding FILES mode, which would be the same as the current behaviour for loose arguments, so the following would be equivalent:

roslint_cpp(FILES src/main.cpp src/foo.cpp)
roslint_cpp(src/main.cpp src/foo.cpp)

In this scenario, the follow calls would also be equivalent:

roslint_cpp(DIRECTORY .)
roslint_cpp()

Example failure case + fix here: moveit/moveit_msgs#8 (comment)

Reported in #12.

It currently suggests using rand_r instead of rand since that should be more thread-safe. However, rand_r is deprecated according to the man page. It instead recommends using drand48_r. It looks like it would be fairly easy to update in the code.

Would a PR that updates cpplint.py and pep8.py to newer versions be accepted? Thanks!

To eliminate files from linting (eg, tests, files which come from elsewhere, etc).

Should support individual files or globs.

Accessors for structs should be indented the same as they would in classes (not at all). However, cpplint warns that they should be indented one space. This can likely be updated the same way it was for classes.

Given:

// Copyright 2015 Alex Henning

struct A
{
public:
}

Result:

structs.cpp:5:  public: should be indented +1 space inside struct A  [whitespace/indent] [3]
Done processing structs.cpp
Total errors found: 1

There are currently no rosdep keys defined for python-pylint or python-cpplint, which are needed to resolve the roslint package build dependencies.

Plus, these probably need to use the transitive <run_depend> instead, since they are needed at build time by packages that depend on roslint. (With package format 2, that will become a <build_export_depend>, see the rep-0140 draft).

It looks like roslint does not have a source entry for Noetic and is directly blocking 54 other repos. Mind adding a source entry for it? Will the noetic release use the master branch?

A source entry signals to other packages it is being worked on, and makes it possible for downstream maintainers to use rosinstall_generator to download their dependencies while they work on Python 3 incompatibilities.

I didn't see any Python 3 syntax errors, and all the tests appeared to pass using Python 3 on Debian Buster. Is roslint already compatible with Python 3?

As discussed here: 0136f53

Use the python facilities provided by rosunit for this, rather than a template baked into the bash wrapper.

Reported in #12

Greetings! I'd love to release xacro into Kinetic, but it depends on roslint. Can you do a Kinetic release? Thanks 🍔🍔🍪

I think we can share some syntaxes for package.xml.

like
CMakeLists.txt

• alphabetical order for msgs and srvs

package.xml

• alphabetical order for build_depend and run_depend
• Order: buildtool_depend -> build_depend -> run_depend -> test_depend

Any idea?

Once the current Indigo sync occurs:

• Tag roslint 0.10.0.
• Indigo release.
• Jade release.
• Announcement to ros-users.

I couldn't find how to run roslint with catkin_tools at ROS wiki
How can I run it?

with

roslint_cpp()

in CMakeLists.txt?

needed for: #1

The default set (in pylint) is way too pedantic, imo.

When "roslint_add_test()" is used in the CMakeLists.txt, "catkin_make run_tests" is able to execute roslint and populate the errors in the RESULTS_FILE.
However, this does not break the test and catkin_make completes without error.

In my code I have a do/while loop as follows:

do
{
    // do something ....
} while (!msg_received);

I get the following error from lint:

} should be on a line by itself  [whitespace/braces] [4]

But if I move the while statement to a new line, I get the next error:

Empty loop bodies should use {} or continue  [whitespace/empty_loop_body] [5]

Is there anyway to solve this issue?

A catkin linter is already being tracked here: ros/catkin#153

So for now, this is blocked on the availability of that tool.

The following code:

server = registerServer("server_name", {State::First, State::Second});

where we pass C++11 bracer-init-list as the second argument fails to pass the CheckBracers. The special case where both bracers happen to be on the same line fails because only one character is expected after the closing bracer }, but in our case we have 2 characters });

Chaninging the line

if Match(r'^(.*){(.*)}.+$', line): 

would probably solve the issue, but as I am not an expert in writing linters, it could potentially break other stuff.
Any suggestions ?

I need to write some code that uses std::transform, like this:

  std::transform(rows.begin(), rows.end(), std::back_inserter(concepts), [this](const mysqlx::Row & row)
  {
    return Concept(row[0], *this);
  });

This will generate } should be on a line by itself [whitespace/braces] [4]. If I do this though, I'll just get closing ) should be moved to the previous line [whitespace/parens] [2].

Check for well-formedness, sane indentation.

This is a friendly reminder that the Melodic buildfarm is now available. It would be great to have a release of roslint into Melodic, as it is currently directly blocking 18 packages and indirectly blocking 28 packages (http://repositories.ros.org/status_page/blocked_releases_melodic.html). Thank you!

It might be useful to add basic checks on the package.xml for a package.

Particularly, I've seen a few packages use <license></license> which doesn't cause any errors, but makes RPM generation fail, and is really against the package.xml format rep: http://www.ros.org/reps/rep-0127.html#license-multiple-but-at-least-one

For the packages who did this, they left it blank because the code was in public domain and was not licensed. I recommended using <license>Public Domain</license>, and all so far have adopted it.

There could also be checks for things such as properly formatted E-mail addresses for authors and maintainers, and properly formatted (and non-empy) URLs.

With the v2 package.xml spec rolling out soon, there will likely be more nuances like this that a lint tool should check.

Great work here!

Creating an output file would be useful for users.

Such a target should cause the lint command only to run again when its dependencies change.

As pointed out in #12, some ROS code which follows PCL triggers a ton of these.

Especially important for:

• situations where a warning is acknowledged, but not actually valid.
• maintainers who don't have the resources to clean everything up now, but would like to broadly enforce no new violations, particularly in conjunction with using Travis to pre-screen PRs.

Apart from catkin_lint, primarily check for:

• two-space indentation.
• lowercase function and macro names.
• uppercase variable names.

This is in direct contradiction to the ROS style guide.

For example:

roslint_python(src/mypkg/foo.py AS_TEST lint_python)

We would create the roslint and roslint_mypkg targets as usual, but also create a run_tests_mypkg_lint_python target, depended upon by run_tests. The idea would be that you could optionally have the linter check run as part of the CI's run_tests build.

On the implementation side, we should be able to call catkin_run_tests_target to wire it up, though you need to provide xunit xml output for that; that aspect of this intersects with #9. (And there may need to be transformation stage in there, to coerce the line-by-line output into the requisite XML.)

Can roslint_cpp default glob files add .hpp suffix? or add another option for the function?

The recommended style for #ifndef guards includes the package name

#ifndef PACKAGE_PATH_FILE_H

This tool suggests only PATH_FILE_H

**Unless I'm invoking the roslint tool with the wrong parameters. The documentation isn't super clear on the CLI.

At the very least, could check for valid XML, but perhaps also for deprecated tags, absolute paths, etc.

I cannot see the LICENSE file (explicit, including copyright holders and year). This makes it hard to reuse the code from this repo lawfully.

The current brace check overlooks many instances of the braces being on the same line. This is due to the fact that in line 99 it suppresses any warnings where there is an = sign on the line. However, this occurs in many common cases:

if (i == 0) {
for (int i = 0; i < 10; i++) {
while (i == 0) {

I think the best fix is to check if the { is preceded by a ) in which case it is likely one of the above. I don't think that would occur with a brace initializer.

@mikepurvis Thoughts?

While useful in general, this is needed for checking the unit test return status (#7).

Add cpplint tests in order to allow cpplint.py to be updated more confidently.

Patched GetHeaderGuardCPPVariable method expects to find "include" in a header file's path. When "include" is not found in the file's path this method hangs in a continuous loop:

    var_parts = list()
    head = filename
    while head:
        head, tail = os.path.split(head)
        var_parts.insert(0, tail)
        if head.endswith('include'):
            break

This is directly contrary to the Cpp style guide example: http://wiki.ros.org/CppStyleGuide#Formatting

Hi,

Would it be possible to set a longer limit for the python line size? 80 is really small - even pep8 says that 100 would be ok.

Cheers

Running the linters depends on rosrun, which does not work unless you've sourced the devel space.

Unsure if catkin_find fixes this, or if it will just remain an open issue.