general: rework the way files are included for better maintainability#1607
general: rework the way files are included for better maintainability#1607ruevs merged 3 commits intosolvespace:masterfrom
Conversation
|
@iscgar I like some of this. Splitting out the handles into their own thing is IMHO a good idea. It might be good to separate out their functions as well, but this is a good start. I'm not sure what the point of the "solvespace" namespace is. I presume it's more for the library than the application? Like when should something be in that namespace? |
|
Yes, it's mostly for the benefit of the library, but it's also useful in general to avoid conflicts with global symbols defined by dependencies of SolveSpace. The need to put even source files that are not used by the library into the namespace was mainly for consistency, as otherwise we'd still need to add Regarding the handles, I did it mainly to reduce the dependance on |
|
@rpavlik can you weigh in on this namespace stuff? |
f3f8959 to
a8f2267
Compare
|
@ruevs do you want weigh in before I merge this? |
I don't remember why I originally did it this way, but I think it was in order to reduce the number of forward declarations needed to get the code to work properly and to simplify the definition dependency, since Additionally, this branch is also the base of my (still WIP) |
|
@iscgar in this case may I suggest that in this PR we commit a sketch.h that is as close as possible to cb561ba to avoid confusion. I've made the necessary changes here https://github.com/ruevs/solvespace/commits/iscgar/clean-up-file-inclusion/ as a new commit. (it may better to interactively rebase and change 47a2e98 but I'll do it if you are OK with it. Or you can do it). If/when you make a PR revamping |
a8f2267 to
09bc2a1
Compare
Before this change, every header file was included in `solvespace.h`, inside the `SolveSpace` namespace. This meant that no header file could itself include a header file, unless it was included from `solvespace.h` as well, as otherwise standard library symbols would be injected into the `SolveSpace` namespace and used from there, causing compilation and linking issues. It also meant that no header file could be included on its own, because its dependencies were only satisfied by the inclusion order in `solvespace.h`, which means that the only way to include anything was to just include `solvespace.h`. Change it so that the headers files are properly included at the top of `solvespace.h`, outside the `SolveSpace` namespace. As a consequence of this change, each header file needs its dependencies to be explicitly satisfied, so do that. This also required shuffling the order of a few definitions in some files, as well as extracting some small definitions into their own files, so they could be depended upon without causing a circular inclusion. While at it, remove the `using namespace SolveSpace` declaration from `solvespace.h` in order to avoid polluting the global scope, and add the needed namespace declaration in every place that required it.
This makes it easier to avoid including dsc.h when it's not needed.
Only include files that are actually needed, instead of using the huge dependency that is `solvespace.h`.
09bc2a1 to
68620cf
Compare
I've already made a similar change in my branch, and I'm only now seeing that I forgot to push it. Sorry about that. My changes are a bit different than yours, and I think they are reasonably minimal, but I'm fine with going with your changes, as the main thing I care about is the inclusion dependencies, and they're not affected by these changes.
Sure thing. |
|
The macOS failure is caused by Homebrew installing CMake 4 by default. We can pin the CI to the last supported version, or just bite the bullet and update the dependencies. I already updated the other dependencies in #1611, and I have updated versions of cairo and pixman locally, but becuase they are forked in the SolveSpace organsation it's not clear to me how I should contribute those changes. The reason is that the convention seems to be to branch from a release and apply the SolveSpace patches in the side branch, which is impossible to do with a PR flow. Any ideas on how to tackle this are welcome. |
This is good - it avoids all but one forward declaration with minimal rearrangement. There is a little bit more clean up possible in Rebased here: |
The idea behind adding those include directives was to make |
|
I'm convinced :-) Merged. |
I forgot about them. Done here: #1624 |
Before this change, every header file was included in
solvespace.h, inside theSolveSpacenamespace. This meant that no header file could itself include a header file, unless it was included fromsolvespace.has well, as otherwise standard library symbols would be injected into theSolveSpacenamespace and used from there, causing compilation and linking issues.It also meant that no header file could be included on its own, because its dependencies were only satisfied by the inclusion order in
solvespace.h, which means that the only way to include anything was to just includesolvespace.h.An additional annoyance for developers was that code completion tools broke because of this inclusion pattern, making working on SolveSpace a tad more difficult.
This PR introduces as minimal changes as possible to make it so that the headers files are properly included at the top of
solvespace.h, outside theSolveSpacenamespace. As a consequence of this change, each header file needs its dependencies to be explicitly satisfied, so do that. This also required shuffling the order of a few definitions in some files, as well as extracting some small definitions into their own files, so they could be depended upon without causing a circular inclusion.This is the latest iteration of the change that I mentioned here, after working with it in my local branches and tweaking it.