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

Closed
wants to merge 5 commits into from
Closed

WIP/RFC: Iteration #475

wants to merge 5 commits into from

Conversation

rpavlik
Copy link
Contributor

@rpavlik rpavlik 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
Copy link
Contributor

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

@rpavlik rpavlik force-pushed the iteration branch 2 times, most recently from 05b4dac to adaf9b6 Compare September 10, 2019 23:27
@rpavlik
Copy link
Contributor Author

rpavlik 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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants