Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP/RFC: Iteration #475

Open
wants to merge 6 commits into
base: master
from

Conversation

@rpavlik
Copy link
Contributor

commented Sep 9, 2019

IndexRef is like a pointer, except it just stores a pointer to the container
and an index. Any invalid (out of range) IndexRef is equal to any other.
IndexIterator is an iterator over all IndexRef values for a container,
used to have a range-for loop where not only every loop,
but every access through the iterated value, gets looked up based
on index.

If this looks appealing I'll move the code out of the main
consuming CPP file into someplace where the other files can use it. It is coded so that the other places/excuses for not using range-for can also be accommodated (you can get the current index, and can also indicate you'd like to start at something other than element 0)

Since it provides pointer-like behavior using indices, this eliminates/prevents the entire class of bugs like #468: it makes the caching of a pointer redundant, so migrating to using this fixes it all without making the code look more noisy. (I only migrated one file's commented for-loops.)

(It can also be used without range-for in this way - less elegant, maybe, but it works:

        for(i = 0; i < SK.constraint.n; i++) {
            Constraint *cs = &(SK.constraint[i]);
            cs->DoStuff();
        }

becomes

        for(i = 0; i < SK.constraint.n; i++) {
            IndexRef<IdList<Constraint, hConstraint>, Constraint>  cs{SK.constraint, i};
            cs->DoStuff();
        }

We could simplify that type a good deal, but that's code that works now.)

I wish iteration/ranges/iterators didn't require quite so much noise to create, but in any case, that's what is required for now. Most confusing part of this is the middle level of iterator that doesn't actually point to the array, but instead just keeps track of an IndexRef and returns that instead of a pointer or reference.

@whitequark

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

I'm not sure this feature is pulling its weight in terms of required churn.

rpavlik added 2 commits Sep 10, 2019
Index iteration: allow avoiding invalidated pointers entirely.
IndexRef is like a pointer, except it just stores a pointer to the container
and an index. Any invalid (out of range) IndexRef is equal to any other.
IndexIterator is an iterator over all IndexRef values for a container,
used to have a range-for loop where not only every loop,
but every access through the iterated value, gets looked up based
on index.

@rpavlik rpavlik force-pushed the rpavlik:iteration branch from 6abbf07 to 05b4dac Sep 10, 2019

@rpavlik rpavlik force-pushed the rpavlik:iteration branch from 05b4dac to adaf9b6 Sep 10, 2019

@rpavlik

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

In case this is any more appealing, I split the file out, simplified, and re-worked it for minimum much lower churn.

(In any case, I'm going to hang on to this bit of code myself - it's a handy little capability 😁 )

hEntity face = Entity::NO_ENTITY;

Vector p = ss->PointAt(0, 0),
n = ss->NormalAt(0, 0).WithMagnitude(1);
double d = n.Dot(p);

if(i == is || i == (is + 1)) {
if(ss.GetIndex() <= (is + 1)) {

This comment has been minimized.

Copy link
@ruevs

ruevs Sep 11, 2019

Contributor

This is not exactly the same code. But it does the same, because the loop starts from is.
The old code makes it a bit more obvious that you want the first two elements.

This comment has been minimized.

Copy link
@rpavlik

rpavlik Sep 11, 2019

Author Contributor

yeah, I wasn't sure whether I should just make an i variable to leave this code unmodified, replace both uses of i with ss.GetIndex(), or do this. Replacing both looked long and unwieldy so I did this, but any of those would work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.