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?
There was a problem hiding this comment.
This file is to provide a convenience header for users to include rather than having to know which ones they really want. For example, if they want our step function and all of the structs that it depends on, they would otherwise have to include 5 different headers. Before this change, they would have gotten them all from including step.hpp, but this is violating the guideline of including what you use and sets them up for breakage if we make any changes.
Internally, we should not use this header and continue including everything we want directly.
| 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
There was a problem hiding this comment.
Forward declarations are good for decreasing compile times and binary size, but are also for decoupling our code and ensuring that we are doing a good job of managing our dependencies.
an example: in our unit tests, we created and manipulated the Constraints and StepParameters structs. However, we never actually included them, instead getting their code transitively from Step.hpp. A change in our code layout broke our tests and could break user code.
Their cautionary tales against the forward declaration seem to pertain to using a forward declaration within an implementation. I agree that this is bad. I don't think they apply when using forward declarations when defining your interface, which is common practice.
| ); | ||
| } | ||
| } | ||
| 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&);
There was a problem hiding this comment.
I don't think we have a policy for this yet. I don't have a strong opinion about which I prefer.
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? |
Maybe, but I don't think so. This split already slows down our compile times from scratch significantly. Truth be told, I'm not even sure if this change is a good idea (I put it together hoping it would be a massive, unqualified win). If we ever decide to do any templating on these functions, we'll have to pull them back into header files, for example. This break location was also chosen because it has the best chance of insulating our users from long compile times (they will mostly use the functions in the |
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.