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

Code Cleanup #79

Merged
merged 6 commits into from
May 26, 2020
Merged

Code Cleanup #79

merged 6 commits into from
May 26, 2020

Conversation

beetleskin
Copy link
Contributor

@beetleskin beetleskin commented Apr 19, 2020

Fixes #76 and #75

Test Plan:

  • find missing violations of http://wiki.ros.org/CppStyleGuide (might be easier with a clang-tidy config we could also use instead of my manual refactoring? maybe as a next step ..)
  • confirm no regression in functionality
  • confirm compilation on clean docker (we need a CI .. ;))

@schra
Copy link
Collaborator

schra commented Apr 22, 2020

I'll look at this once we have merged your other PRs :)

@schra
Copy link
Collaborator

schra commented Apr 28, 2020

Just as a reminder from #72, could you maybe add this small change in this PR too?

When clearing the topic input, the error will be "No map received yet" - maybe we should change that to "No NavSatFix message received yet"?

@beetleskin
Copy link
Contributor Author

lets do that after review/concerns fixed - just remind me ;)

@beetleskin
Copy link
Contributor Author

#77 is also part of this PR, I'll rebase after #77 is merged

@beetleskin
Copy link
Contributor Author

beetleskin commented May 1, 2020

@schra are we using a fork of https://github.com/davetcoleman/roscpp_code_format ? what about clang-format and camelCase vs. under_score? Can one set that in the config? Or is that only something clang-tidy can do? Should we add a clang-tidy config? Is there one for ROS? I only know the one from moveit (https://github.com/ros-planning/moveit/blob/master/.clang-tidy).
So many questions? :)

Copy link
Collaborator

@schra schra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :)!

Please rebase again on #77 (I did a small change there)

You missed two variables ;)

src (cleanup) $ cgrep --var --regex  ".*[a-z][A-Z].*" $(find -name "*.cpp" -or -name "*.h")

/home/andre/rviz_satellite/src/tile_cache.h:163:21:        TileId const toFind{ area.center.tile_server, { xx, yy }, area.center.zoom };
/home/andre/rviz_satellite/src/aerialmap_display.cpp:489:19:      TileId const toFind{ center_tile_->tile_server, { xx, yy }, center_tile_->zoom };

src/aerialmap_display.cpp Show resolved Hide resolved
src/aerialmap_display.cpp Outdated Show resolved Hide resolved
src/aerialmap_display.cpp Show resolved Hide resolved
src/aerialmap_display.cpp Show resolved Hide resolved
src/aerialmap_display.cpp Show resolved Hide resolved
src/mercator.h Show resolved Hide resolved
src/aerialmap_display.cpp Outdated Show resolved Hide resolved
src/aerialmap_display.h Outdated Show resolved Hide resolved
@schra
Copy link
Collaborator

schra commented May 5, 2020

@schra are we using a fork of https://github.com/davetcoleman/roscpp_code_format

yes

what about clang-format and camelCase vs. under_score? Can one set that in the config? Or is that only something clang-tidy can do? Should we add a clang-tidy config? Is there one for ROS? I only know the one from moveit (https://github.com/ros-planning/moveit/blob/master/.clang-tidy).

That's a really good point. I'll add a remark about this in #78 And yeah, I think you can only do that with clang-tidy :)

We try to follow the ROS' coding style guideline (http://wiki.ros.org/CppStyleGuide).

- replace any camelCase variable with under_score (as default in rviz)
- replace any camelCase file name with under_score (as default in rviz)
- refactoring, doc improvement
- no functionality changed
@beetleskin
Copy link
Contributor Author

You missed two variables ;)

inferior humans ..

I rebased, and fixed concerns - the new commit should be merged into the previous one; but I'd keep the others as they are

Copy link
Contributor Author

@beetleskin beetleskin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a shame that it's so hard to keep track of unused includes in C++ ..

@schra schra merged commit 5431732 into nobleo:master May 26, 2020
@schra
Copy link
Collaborator

schra commented May 26, 2020

LGTM now, thanks again :)

@beetleskin beetleskin deleted the cleanup branch May 26, 2020 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IncludeWhatYouUse
3 participants