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

Refactor and cleanup of codebase #75

Conversation

gavanderhoorn
Copy link
Member

@gavanderhoorn gavanderhoorn commented Apr 15, 2016

Cleanup of the current codebase (indigo-devel @ a80155e).

Header (include) dependencies have been reduced as much as possible across all files and all property classes have been split into separate files. This greatly increases locality of changes as it reduces rippling, leading to much shorter edit-compile-run-test cycles when developing.

@gavanderhoorn
Copy link
Member Author

@Levi-Armstrong: could you take a look at this one as well? It's probably less involved than it seems. There are no functional changes.

@gavanderhoorn
Copy link
Member Author

I'll merge this one before #73 and #72, which will have to be rebased.

@Levi-Armstrong
Copy link
Contributor

+1; Looks great, OK to merge!

This is a significant refactoring, which is intended to reduce the nr of include
dependencies in all source and header files. Forward declaration of classes is
used as much as possible, and includes have been moved to the cpp files as much
as possible as well.

This greatly increases locality of changes, resulting in shorter compilaton
times whenever changes have been made to any of the class definitions as it
avoids rippling significantly. Initial compilation is slightly slower, but that
is considered an acceptable trade-off.
 - sort depends, source and header lists
 - don't export include directories (this is a top-level application)
 - fix indent (minor)
@gavanderhoorn gavanderhoorn merged commit 2e05514 into ros-industrial-attic:indigo-devel Apr 19, 2016
@gavanderhoorn
Copy link
Member Author

Ended up doing it in reversed order: first #73 and #72, then this one.

@Levi-Armstrong: thanks for taking a look 👍.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants