Linting for rviz_rendering #17

Martin-Idel · 2017-08-31T11:23:34Z

This pull request adds liniting to rviz_rendering and fixes all linting issues to have a clean build.

Closes #1

wjwwood · 2017-08-31T23:34:06Z

rviz_rendering/include/rviz_rendering/logging.hpp

+//    copyright notice, this list of conditions and the following
+//    disclaimer in the documentation and/or other materials provided
+//    with the distribution.
+//  * Neither the name of the Open Source Robotics Foundation, Inc. nor the names of its


I don't think this is the right change because Willow Garage's name should still not be used (nothing has changed in that respect).

The updated 3-clause bsd template uses a more general statement here:

Neither the name of the copyright holder nor the names of its contributors may be used to endorse
-- https://choosealicense.com/licenses/bsd-3-clause/

We should probably update to that instead.

Also there is a pull request I am looking into that should allow us to use the block comments rather than // comments, letting us have smaller diffs:

ament/ament_lint#82

So if you want to merge this, for now I would suggest disabling the copyright linter and wait for that pr to get merged before trying to enable it again. Otherwise I am going to wait to merge this pr until we have that sorted out because I really don't want to have a diff like this in all the files we port.

I agree - it would be much better to have ament_lint copyright check be able to work with block comments. If this happens soon, I'll be inclined to wait for it. Otherwise, I can also disable it if you want.
Regarding the change, I wasn't sure (that's why it's in a separate commit) - it just seemed logical because the file has no connection whatsoever to Willow Garage.

The template I chose was the one recognized by ament_lint - see here: https://github.com/ament/ament_lint/tree/master/ament_copyright/ament_copyright/template

the weird part I only saw now is that the license template is equivalent to the bsd-3-clause you mention, while the license header isn't - and ament_copyright checks for the header.

wjwwood · 2017-08-31T23:35:58Z

rviz_rendering/src/rviz_rendering/ogre_logging.cpp


 void OgreLogging::useLogFile(const std::string & filename)
 {
  preference_ = FileLogging;
-  filename_ = filename;
+  filename_ = filename.c_str();


This is not safe, as the lifetime of filename is not guaranteed to be longer than filename_, that's why I just disable the linter on this one until that is resolved some other way. You can either leave the NOLINT or fix it another way.

wjwwood · 2017-08-31T23:36:55Z

rviz_rendering/src/rviz_rendering/ogre_logging.cpp

@@ -124,6 +127,7 @@ void OgreLogging::configureLogging()
  if (preference_ == StandardOut) {
    ll.min_lml = Ogre::LML_NORMAL;
  }
+  delete log_manager;


I don't believe this to be correct either. I think the logger will be held by Ogre::LogManager::getSingletonPtr()afterwards.

Looking into it, you're absolutely right. I find this quite weird style, though: I would've expected there to be some kind of static factory method instead - if I new an object, this should mean I'm also responsible to delete it. I guess we have to disable cppcheck for this newcall then. I'll look into it.

wjwwood · 2017-08-31T23:38:05Z

rviz_rendering/src/rviz_rendering/resource_config.cpp

@@ -60,7 +61,7 @@ get_resource_directory()
 void
 set_resource_directory(const std::string & resource_directory)
 {
-  __resource_directory = resource_directory;
+  __resource_directory = resource_directory.c_str();


This is not safe either, for the reasons described above.

wjwwood · 2017-08-31T23:38:11Z

rviz_rendering/src/rviz_rendering/resource_config.cpp

@@ -72,7 +73,7 @@ get_ogre_plugin_directory()
 void
 set_ogre_plugin_directory(const std::string & ogre_plugin_directory)
 {
-  __ogre_plugin_directory = ogre_plugin_directory;
+  __ogre_plugin_directory = ogre_plugin_directory.c_str();


- we don't use ament_lint_common because copyright checks fail - we want to wait for changes in ament_copyright before fixing

- only one error: memory leak of log_manager

- SdkQtCameraMan is from OgreBites and not from rviz - No change in functionality

Martin-Idel · 2017-09-01T14:13:50Z

I updated the pull request making all requested changes (to simplify things, I force-pushed):

we don't use ament_copyright now and all copyright changes were discarded
the cppcheck error for the memory leak in ogre_logging and the cpplint errors for static std::strings are only ignored (no changes)

One additional comment: It would be nicer to still use ament_lint_auto with custom linting tools as specified in https://github.com/ament/ament_lint/blob/master/ament_lint_auto/doc/index.rst - but this didn't work for me, so I had to include the linters manually in CMake.

wjwwood

lgtm, I added a todo for the copyright linter and a comment about the delete-less free.

wjwwood · 2017-09-02T01:12:06Z

One additional comment: It would be nicer to still use ament_lint_auto with custom linting tools as specified in https://github.com/ament/ament_lint/blob/master/ament_lint_auto/doc/index.rst - but this didn't work for me, so I had to include the linters manually in CMake.

Interesting, perhaps it's worth opening an issue on the ament_lint_auto package's repository. This will work fine for now.

mikaelarguedas · 2017-09-02T01:19:18Z

I don't know if it helps but you can bypass a specific linter by tricking cmake into not finding it and keep using ament_lint_auto for everything else (example to disable only the copyright linter here)

* Introduce linters in rviz_rendering - we don't use ament_lint_common because copyright checks fail - we want to wait for changes in ament_copyright before fixing * Fix cppcheck errors - only one error: memory leak of log_manager * Extract SdkQtCameraMan from OgreBites to fix linters - SdkQtCameraMan is from OgreBites and not from rviz - No change in functionality * adding a todo about the copyright linter * add a comment about delete-less new call

Martin-Idel changed the title ~~Feature/linting~~ Linting for rviz_rendering #1 Aug 31, 2017

Martin-Idel changed the title ~~Linting for rviz_rendering #1~~ Linting for rviz_rendering Aug 31, 2017

wjwwood reviewed Aug 31, 2017

View reviewed changes

wjwwood requested changes Aug 31, 2017

View reviewed changes

wjwwood added enhancement New feature or request in progress Actively being worked on (Kanban column) labels Aug 31, 2017

Martin-Idel-SI added 3 commits September 1, 2017 16:02

Introduce linters in rviz_rendering

b5caa6b

- we don't use ament_lint_common because copyright checks fail - we want to wait for changes in ament_copyright before fixing

Fix cppcheck errors

89b9b74

- only one error: memory leak of log_manager

Extract SdkQtCameraMan from OgreBites to fix linters

bfcdc73

- SdkQtCameraMan is from OgreBites and not from rviz - No change in functionality

Martin-Idel force-pushed the feature/linting branch from 0cc10d8 to bfcdc73 Compare September 1, 2017 14:06

wjwwood added 2 commits September 1, 2017 18:04

adding a todo about the copyright linter

12ca19b

add a comment about delete-less new call

e30ea54

wjwwood approved these changes Sep 2, 2017

View reviewed changes

wjwwood merged commit 9d62823 into ros2:ros2 Sep 2, 2017

wjwwood removed the in progress Actively being worked on (Kanban column) label Sep 2, 2017

Martin-Idel-SI mentioned this pull request Oct 18, 2017

RViz segfaults and doesn't end properly #73

Closed

anhosi deleted the feature/linting branch November 9, 2017 16:49

hidmic mentioned this pull request Oct 29, 2020

Ensure rviz_common::MessageFilterDisplay processes messages in the main thread #620

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linting for rviz_rendering #17

Linting for rviz_rendering #17

Martin-Idel commented Aug 31, 2017 •

edited

Loading

wjwwood Aug 31, 2017

Martin-Idel Sep 1, 2017

wjwwood Aug 31, 2017

wjwwood Aug 31, 2017

Martin-Idel Sep 1, 2017 •

edited

Loading

wjwwood Aug 31, 2017

wjwwood Aug 31, 2017

Martin-Idel commented Sep 1, 2017

wjwwood left a comment

wjwwood commented Sep 2, 2017

mikaelarguedas commented Sep 2, 2017

Linting for rviz_rendering #17

Linting for rviz_rendering #17

Conversation

Martin-Idel commented Aug 31, 2017 • edited Loading

wjwwood Aug 31, 2017

Choose a reason for hiding this comment

Martin-Idel Sep 1, 2017

Choose a reason for hiding this comment

wjwwood Aug 31, 2017

Choose a reason for hiding this comment

wjwwood Aug 31, 2017

Choose a reason for hiding this comment

Martin-Idel Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

wjwwood Aug 31, 2017

Choose a reason for hiding this comment

wjwwood Aug 31, 2017

Choose a reason for hiding this comment

Martin-Idel commented Sep 1, 2017

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood commented Sep 2, 2017

mikaelarguedas commented Sep 2, 2017

Martin-Idel commented Aug 31, 2017 •

edited

Loading

Martin-Idel Sep 1, 2017 •

edited

Loading