Split functions within the step folder into .hpp and .cpp files #279
Conversation
This change should improve compile times, especially with incremental builds
There was a problem hiding this comment.
I don't think forward declaring the structs in the header files is a good idea. I understand that it allows the headers to be decoupled, but then if you want to actually use those headers you have to track down the origin of each type that has been forward declared. This makes the library harder to use and is likely to frustrate the users. I'd recommend going ahead and referencing the appropriate includes.
We can add more lines to this file depending on how people end up interfacing with the code, but this is a good starting point.
ddement
force-pushed
the
compile_step
branch
2 times, most recently
from
a6b5d69
to
5c8ffab
Compare
|
I've added the top-level Let me know if this looks good or if you'd still prefer to include in the lower level headers. |
There was a problem hiding this comment.
What is the intent for this file?
| struct Solver; | ||
| struct Constraints; |
There was a problem hiding this comment.
This makes me nervous -- why are we using forward declarations here? I think the risks outweigh the compile-time savings here (e.g. unit testing this -- would there be surprises?). For what it's worth, Google's style guild also discourages it.
https://google.github.io/styleguide/cppguide.html#Forward_Declarations
| ); | ||
| } | ||
| } | ||
| void AssembleConstraintsMatrix(Solver& solver, const Constraints& constraints); |
There was a problem hiding this comment.
Do we have a policy on declaring parameter names (which are not primitive) in the headers-only files? For example, would the following be cleaner?
void AssembleConstraintsMatrix(Solver&, const Constraints&);
Just wondering if any other parts of openturbine might benefit from such a split -- what are your thoughts? Is maintaining a mix of header-only files + splitting them out as appropriate the optimal path? |
This commit should improve compile times for incremental builds quite significantly, depending on where you're making your changes. Note that it seems to slow down building from scratch by a little bit (about 20-50%).
The functions in this folder mark the transition from a high level "pass around the structs" workflow to actually calling the kernels that do the work, so they are a reasonable place to provide the split to get the most "bang for our buck" with incremental compile time improvements.