Hopefully-Uncontroversial cleanups#413
Conversation
|
updated to fix a style issue. |
whitequark
left a comment
whitequark
left a comment
There was a problem hiding this comment.
Thanks. I've wanted to make some of these changes myself for similar reasons but never got around to it, so I'm definitely interested in this patchset.
| // Get some arbitrary point in the sketch, that will be used | ||
| // as a reference when defining top and bottom faces. | ||
| hEntity pt = { 0 }; | ||
| // Not using range-for here because we're changing the size of entity in the loop. |
There was a problem hiding this comment.
Oh of course that's why my own range-for patch (in a local branch that got buried years ago) didn't work. Iterator invalidation, the second worst thing in C++ that "modern" C++ does absolutely nothing to help with...
There was a problem hiding this comment.
Can we add iterators that don't get invalidated when inserting into the collection, maybe? Store an index as well as pointer to the element.
There was a problem hiding this comment.
maybe as "future work". I did have some patches that make an IdListAdditions type, which basically queues up things to add so it can be bulk-added at the end via a merge. Would also avoid invalidating iterators if iterating an idlist, but not sure the clarity tradeoff is worth it. (I reverted the patch on my dev branch after doing the port to std::vector as a backend, since that gave the equivalent performance improvement. Think it was allocation behavior that was gettting me.)
There was a problem hiding this comment.
Here's that branch rebased on this, and trimmed. https://github.com/rpavlik/solvespace/commits/idlist-additions
See in particular this: rpavlik@93f5e41 I only did this in the few places where I saw it coming up a lot on profiling data, but I think this could be used to avoid iterator invalidation in loops as well - instead of adding directly, add to an "additions" object, then "MoveIntoList" after iteration is complete.
|
Cool, thanks. Will rebase with the changes suggested. |
Also add comments about indexing and when we don't use range-for.
|
OK, rebased the remaining changes, tried to make my commit messages more like yours and combine some commits. |
|
|
||
| inline int WRAP(int v, int n) { | ||
| template<typename T> | ||
| inline T WRAP(T v, T n) { |
There was a problem hiding this comment.
(I think you missed my earlier question.) I don't understand this change--what's the point of making WRAP generic if it's only used with ints anyway and you need to cast it each time?
There was a problem hiding this comment.
Ah, I did miss your question. To be honest, I have no idea, that's the risk of me leaving commits un-submitted for so long.
|
Merged f7f0a93, adding a similar assert for SMesh as well. |
|
Closing since most of this got in |
|
I actually very much want for loops, that will enable some IdList optimizations, see #430. |
|
Ah, you've opened #428. Sorry for the noise. |
2 participants
Went through my old branches and pulled out the commits that look reasonably safe and innocuous. (My switch to use std::vector inexplicably is causing test failures, and my performance improvement to bulk-add to non-vector collections is not very pretty.)
All tests pass on Debian Buster. Feel free to cherry-pick if some of them don't appeal to you - they're mostly independent or in brief sequences (e.g. the CountIf and GetNumConstraints changes are related, the WRAP changes are related, the rest are pretty independent though I don't know what would happen if you re-ordered the "convert to range-for" commits.)