On its own this change is quite useless, but it's not just for the sake of change.

The reason for this change is that I'm working on improving List<T> and IdList<T>, and due to the way things are included in solvespace.h (inside the SolveSpace namespace), I've been bit by incomprehensible compilation errors when including standard library headers in dsc.h. dsc.h is already including some standard library headers, and the only reason it works is a happy accident caused by the fact that solvespace.h is already including them before including dsc.h. In addition, dsc.h is also including solvespace.h, and while this works in most compilers thanks to them identifying the include guard pattern, some compilers may choke on this circular dependency and end up in an endless recursion.

In trying to fix it and untangle the inclusion of everything through solvespace.h (which also means that any change in a header file triggers a recompilation of everything), so that every file only includes the header files it actually needs instead of bringing everything in through solvespace.h, I've been hit by this name conflict and decided to try and send this small patch instead of doing it as part of an unrelated PR.

While I don't mind changing the test definitions instead, I also totally understand your point of view regarding changes to the code base in general. If that's a change you'd prefer to avoid, I'm fine with closing this PR.

@ruevs I'm kind of in favor of refactoring the header files if it actually improves compile times and makes sense. I understand the arguments against change too. But if we change them I feel like it should be more of a one-time large change just to restructure headers and not a little change here or there. What do you think?

Sometimes new contributors are put off by the code structure, but OTOH the core is complex and people who can make good progress in there seem to be able to look past the strange organization. I find the separation of code based on function rather than classes interesting - ratpoly.cpp is still my favorite file and it contains functions for curves and surfaces, even though those are in different classes it makes sense. But the one giant header is kind of icky.

As for CHECK_TRUE and CHECK_FALSE I don't really want to lengthen the names... Maybe "check" is the wrong word in the test suite?

@iscgar What are you trying to do with List and IdList ? Those are very key structures. I don't really like them personally, but they are key to a lot of Solvespace internals.

@phkahler this is off-topic for this PR, but I suppose I only have myself to blame for bringing it up here 😅

in List<T>, I'm converting it into a proper container that doesn't require the manual memory management (which leaks into call sites as well, requiring careful consideration when copying and clearing the list, in order to avoid leaking memory or accidentally double freeing). While it might be useful as a way to avoid making deep copies, it makes code that (ab)uses this fact harder to maintain (besides, that ship has sailed with the conversion of IdList<T> in the past from manual memory management into the use of std::vector as the underlying storage, so users of types containing it are already making deep copies even if that was not the original intent).

In order to ensure that no deep copies are accidentally introduced and hurt performance, while working on it I temporarily deleted the copy constructor and copy assignment operator of List<T> as a way to track down all the places where such copies were made, and converted them to std::move() where applicable, or simple references where that was the intention (as I mentioned in #1536 (comment) ).

For IdList<T>, I'm working to reduce its memory usage, especially when items are deleted in bulk, and fixing the issue that causes it to be unable to actually call the destructor of its items on removal (which is somewhat alleviated by the use of the clunky .Clear() interface, which requires all items stored in it to implement it. This same issue is present in List<T> before my changes). I've additionally made some minor performance improvements to item deletion, but as they are minor, I wouldn't use that as a reason for accepting the changes.

As a data point, while still a WIP, when running the tests (in release mode) with my changes, I'm seeing ~10% reduction in memory usage of List<T> and IdList<T>.

@ruevs, based on your and @phkahler's messages above, I'm closing this PR. Would a refactoring of the header files (which will rename the test definitions instead) be acceptable?

In trying to fix it and untangle the inclusion of everything through solvespace.h (which also means that any change in a header file triggers a recompilation of everything), so that every file only includes the header files it actually needs instead of bringing everything in through solvespace.h

For cross reference #359

caused by the fact that solvespace.h is already including them before including dsc.h. In addition, dsc.h is also including solvespace.h

Indeed. See here: #1503

Do not Interpret the above as comments from me. I'm just adding some cross references from my phone :-)

Wasn't going to interpret it that way :)
Thank you for taking the time to post these cross reference comments. It really helped me get the context for more than one issue that I was looking at for past ideas.

A perhaps less controversial idea: instead of refactoring the header files, I'd love to begin by just changing the current way things are included in solvespace.h inside the SolveSpace namespace and move them to the top of the file, so that at least header files could individually include standard library headers without worrying about the way they are being injected into the SolveSpace namespace. Once this is done, and the circular include isn't needed anymore, we can continue to discuss if a refactor of the header file is desirable.

Be careful of trying to refactor those containers to remove manual memory management - there is a list of my previous attempts to do so around here somewhere... if nothing else in the branches on my fork. 😉 Harder than it looks because there is some usage of them as a pool, etc.

I do also strongly dislike the weird header include namespace inject thing. As long as it can be done neatly I would be in favor of fixing that.

Be careful of trying to refactor those containers to remove manual memory management - there is a list of my previous attempts to do so around here somewhere... if nothing else in the branches on my fork. 😉 Harder than it looks because there is some usage of them as a pool, etc.

I'm certainly being careful, or at least trying to :)
I've been checking every usage of them throughout the code and fixing things as I go (which is why it's still a WIP). Thanks to @ruevs's comment in #1536 (comment) I have a bunch of references to draw inspiration from and note down pitfalls, and I also checked all of the branches in your fork that touched the containers even before that (your comment in 2bc945b was what drew my attention to the fact that my changes could affect code that relies on the copy being a shallow one).

I do also strongly dislike the weird header include namespace inject thing. As long as it can be done neatly I would be in favor of fixing that.

I have already made such a change and pushed it into a branch in my fork here. If that's what you had in mind, and if it's something that is deemed acceptable, I'll open a PR.