You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I worked on my first issue here and collected few things that make development uncomfortable.
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.
Inconsistent or mixed tab/space policy.
(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.
The text was updated successfully, but these errors were encountered:
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.
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.
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?
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
I worked on my first issue here and collected few things that make development uncomfortable.
Otherwise developer gets long-term rebuilds after a single change in header.
P.S. I can work on it after zooming.
The text was updated successfully, but these errors were encountered: