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

Development issues #138

Open
fpohtmeh-github opened this issue Aug 4, 2020 · 3 comments
Open

Development issues #138

fpohtmeh-github opened this issue Aug 4, 2020 · 3 comments
Labels
enhancement New feature or request

Comments

@fpohtmeh-github
Copy link
Contributor

I worked on my first issue here and collected few things that make development uncomfortable.

  1. Active usage of includes in headers. Please replace with forward declarations where it's possible.
    Otherwise developer gets long-term rebuilds after a single change in header.
  2. Inconsistent or mixed tab/space policy.
  3. (Minor) src/CMakeLists.txt includes source files only. Please include corresponding header files as well. This will improve project navigation and show header-only classes in project tree.

P.S. I can work on it after zooming.

@DamirPorobic DamirPorobic added the enhancement New feature or request label Aug 10, 2020
@DamirPorobic
Copy link
Member

Thanks for bringing this to my attention.

  1. I didn't think that this would be a problem as our classes are rather small. On the other hand, it's not like have thought about it too much. If you think that there is a noticeable compile time improvement, we should probably go for it.
  2. Sorry for that, might come from me using different machines for developing (Windows, Mac, Linux, different VMs) and not fully setting up my IDE. Tab should be used everywhere.
  3. I'm not sure about this point as it would be additional work to add the header files, also additional work with renaming files as you would have to rename always two files and makes the CMakeList.txt larger and harder to navigate. Most of the CMake based projects that I have seen so far include only the source files or that's at least what I remember seeing. What IDE are you using? Is it possible to switch to a directory based one?

@DamirPorobic
Copy link
Member

looking at your PR, I see that you put your includes into the implementation file. Is this related to 1.?

@fpohtmeh-github
Copy link
Contributor Author

looking at your PR, I see that you put your includes into the implementation file. Is this related to 1.?
Yes, just a good practice to minimize build time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants