Remove unnecessary functions from headers #71
Conversation
|
Note that this PR doesn't contain any actual code changes, because reviewing a 7000 line diff with actual code changes would be cruel. Also, I couldn't figure out the correct recipe to format code. Do you use clang-format? If so, which version? Also, the includes from the header files could possibly be pruned. |
|
I feel obliged to mention that there is one downside to this: is makes the ABI more fragile. Because Actsvg has a lot of STL types on its API boundary, turning that API boundary into an ABI boundary will make the code a bit more fragile to compiler changes. However, I don't think this outweighs the potential benefits. |
There was a problem hiding this comment.
I am super on board with this development. I really dislike myself as well how many of the Acts projects put everything into header files. Generally I just would like to avoid using extern in the code. (And rather use static variables defined inside structs.)
But other than that, I'm very much on the side of this PR.
| template <size_t DIM> | ||
| static inline std::pair<svg::object, svg::object> cluster( | ||
| std::pair<svg::object, svg::object> cluster( |
There was a problem hiding this comment.
Not for this PR for sure, but I wonder if we could move the implementation of this template (or in fact most of the templates) into .cpp files, and just provide implementations for the specializations that make sense. 🤔
|
Indeed, there can be a lot shifted to headers, let's try to do this as first step. |
|
@asalzburger do you use any formatters for this repository? |
Not yet, we should probably just use the This is done, I have merged #74 which adds clang-format and applies it. |
I've noticed that actsvg has most of its code in header files. While this is well managed using the `static` and `inline` keywords, it still leads to some rather undesirable effects. For example, I ran into an issue where a detray CUDA file was crashing because of some code in actsvg, that that kind of very deep dependency is not very maintainable. For that reason, I suggest we move as much of the code from actsvg into sources files. This will help reduce compilation times and memory usage.
stephenswat
force-pushed
the
refactor/src
branch
from
f89f583
to
4259763
Compare
|
Okay, this is ready to go. I suggest we can sort out the extern issue later as it is not so pressing. |
|
Great to have this merged before the summer student arrives! |
I've noticed that actsvg has most of its code in header files. While this is well managed using the
staticandinlinekeywords, it still leads to some rather undesirable effects. For example, I ran into an issue where a detray CUDA file was crashing because of some code in actsvg, that that kind of very deep dependency is not very maintainable. For that reason, I suggest we move as much of the code from actsvg into sources files. This will help reduce compilation times and memory usage.