More cleanups and simplification#428
Conversation
|
Apparently the windows toolchain is too old to deal with defaulting move constructors, etc. Fortunately, this is apparently not necessary anymore, so I just dropped those patches from the series. |
|
Ah ha! I took a different approach to doing the IdListAdditions thing and added a MergeInto method. This let me get rid of nearly all remaining non-range-for over IdLists. See last two commits in https://github.com/rpavlik/solvespace/commits/idlist-merge |
|
That's definitely one way to do it, yup. |
|
OK, this has been rebased and squashed into fewer commits with better messages. I've also included the MergeInto work on IdList, and tweaked the "get rid of qsort" commit to use lambdas for simplicity. This removes nearly all non-range-for loops over containers, at least those that I had detected when I first marked them with comments a year ago. It also addresses the underlying reasons why some couldn't be ported to range-for, such as addition-during-iteration. The other changes are rather standalone - think you can mostly drop any ones you don't care for in the series. The underlying motivation behind these changes is either performance (for those that originated long ago), or improved software structure (for newer ones) - many of these issues were noticed when I attempted to port these containers to being based on std::vector or something else, but the fixes didn't depend on such a port. |
rpavlik
force-pushed
the
simplify6
branch
2 times, most recently
from
6e97784 to
3c4c7cb
Compare
|
OK, undoing the merge-idlists commit because it apparently is resulting in benchmark issues locally? |
|
Thanks for the update! I didn't forget about this PR, was just a bit busy with other projects. |
|
np - I just poked at this again to verify a vscode bug had been fixed, and one thing led to another ;) |
|
@rpavlik I just reviewed all these changes. I am quite happy with how they are. Could you do a rebase and then I'll merge it? |
Also add comments about indexing and when we don't use range-for.
Allows forcing const iteration.
Removes consuming code from the implementation details, easing swap of the underlying container, etc.
Clearer and less error-prone to use standard-supplied algorithms.
Offloads most of the work onto standard algorithms to make it more self-evidently correct.
Pointing out one potential issue with an assert.
Changes resemble those already made to IdList.
Removes static variable usage, permits hiding of the underlying pointer (std::sort uses iterators intead), type safety, etc.
|
oh this is a fun rebase - forgot that I did the "remove all the |
|
(This is still a lot of churn, but it is churn I was prepared to apply anyway, and I am convinced that SolveSpace urgently needs to be modernized if we are to seek healthy external contributors. The current situation where anyone has to learn the unconventional data structures first to do anything is not really sustainable...) |
Allows distancing users from the internal "elem" member. Add Get() and operator[]. Replace direct references to elem. Make elem and elemsAllocated private in IdList/List.
Also modifies a sizeof call.
std::swap is an idiomatic way to do a move.
Was getting segfaults near here with another patch since removed from the branch. Moving these assignments after the memfree means they still have useful data when debugging a crash in memfree.
Counterpart of First(). standard library calls this "back()".
This broke encapsulation and thus caused problems for any deeper changes to List.
|
OK, rebase complete! Thanks for the review! |
|
You've made some excellent contributions to SolveSpace, and as such I've invited you to the SolveSpace organization. For now I would still like all non-trivial changes go through pre-commit code review, but feel free to commit trivial ones directly (typos, broken build, etc.) |
|
Thanks, and thanks for your excellent and prompt reviewing - if only I was as prompt on my PR reviews :) |
Commit 86f20cc (merged in solvespace#428) was marked NFC, even though it didn't just convert loops to ranged-for, but also introduced a functional change of the request pruning logic to prune entities instead. This doesn't make much sense, since the requests will stay, and their entities will simply be regenerated. I'm not sure how it slipped past review, but this was the cause of solvespace#628 and is why it was needed to add workplane deletion code in 586b047, so this commit also reverts that addition.
Commit 86f20cc (merged in solvespace#428) was marked NFC, even though it didn't just convert loops to ranged-for, but also introduced a functional change in the request pruning logic and pruned entities instead. This doesn't make much sense, since the requests will stay, and their entities will simply be regenerated, leading to an infinite recursion and resulting in a crash with stack overflow. I'm not sure how it slipped past code review, but this was the cause of solvespace#628 and is the reason for the additional workplane deletion code that whitequark added in 586b047, so this commit also reverts that code as well.
Commit 86f20cc (merged in solvespace#428) was marked NFC, even though it didn't just convert loops to ranged-for, but also introduced a functional change in the request pruning logic to prune entities instead. This doesn't make much sense, since the requests will stay, and their entities will simply be regenerated, leading to an infinite recursion and resulting in a crash with stack overflow. I'm not sure how it slipped past code review, but this was the cause of solvespace#628 and is the reason for the additional workplane deletion code that whitequark added in 586b047, so this commit also reverts that code as well.
Commit 86f20cc (merged in #428) was marked NFC, even though it didn't just convert loops to ranged-for, but also introduced a functional change in the request pruning logic to prune entities instead. This doesn't make much sense, since the requests will stay, and their entities will simply be regenerated, leading to an infinite recursion and resulting in a crash with stack overflow. I'm not sure how it slipped past code review, but this was the cause of #628 and is the reason for the additional workplane deletion code that whitequark added in 586b047, so this commit also reverts that code as well.
Commit 86f20cc (merged in solvespace#428) was marked NFC, even though it didn't just convert loops to ranged-for, but also introduced a functional change in the request pruning logic to prune entities instead. This doesn't make much sense, since the requests will stay, and their entities will simply be regenerated, leading to an infinite recursion and resulting in a crash with stack overflow. I'm not sure how it slipped past code review, but this was the cause of solvespace#628 and is the reason for the additional workplane deletion code that whitequark added in 61fc345, so this commit also reverts that code as well.
2 participants
This mostly cleans up the remaining pieces I had in flight, plus some extra cleanups. tests pass, the containers aren't very different (still allocated the same), but this lets the "port" to backing by std::vector be fairly simple in addition to making things cleaner and more uniform.