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

[BUG] WeightedContainer: can't iterate while I iterate #413

Open
Suor opened this issue Sep 2, 2024 · 5 comments · May be fixed by #414
Open

[BUG] WeightedContainer: can't iterate while I iterate #413

Suor opened this issue Sep 2, 2024 · 5 comments · May be fixed by #414
Labels
bug Something isn't working

Comments

@Suor
Copy link
Contributor

Suor commented Sep 2, 2024

local cont = WeightedContainer();
cont.add("a", 10);
cont.add("b", 20);
cont.add("c", 30);
cont.add("d", 40);

foreach (k, v in cont) {
    print(k + " " + v + "\n")
    foreach (k2, v2 in cont) {
        print("inner " + k2 + " " + v2 + "\n")
    }
}

Result:

a 10
inner a 10
inner c 30
inner b 20
inner d 40

AN ERROR HAS OCCURRED [arith op + on between 'null' and 'integer']
@Suor Suor added the bug Something isn't working label Sep 2, 2024
@TaroEld
Copy link
Member

TaroEld commented Sep 3, 2024

Yeah I see. This should work:

IStack = [];
...
		if (_prev == null)
		{
			if (this.IItems == null) 
				this.IItems = ::MSU.Table.keys(this.Table);
			this.IStack.push(0)
		}
		_prev = this.IStack.top();
		this.IStack[this.IStack.len() - 1]++;

		if (_prev == this.Table.len())
		{
			this.IStack.pop();
			if (this.IStack.len() == 0)
				this.IItems = null;
			return null;
		}

		return this.IItems[_prev];

@TaroEld TaroEld linked a pull request Sep 3, 2024 that will close this issue
@Suor
Copy link
Contributor Author

Suor commented Sep 3, 2024

It will for nested case, it won't for arbitrary intermixed one. Say I create two generators:

function iter(cont) {
    foreach (k, v in cont) {
        print(k + " " + v + "\n")
        yield [k v]
    }
}

local it1 = iter(cont), it2 = iter(cont);

resume it1;
resume it1;
resume it2;
resume it2;
resume it1; // it2 not finished
resume it2; // bork

@Suor
Copy link
Contributor Author

Suor commented Sep 3, 2024

I think you just need a keyToIndex table to iterate and it could be shared across all generators. The behavior if container changes while iterating still might be undefined, which is fine as long as it not crashes, like original tables.

@LordMidas
Copy link
Member

Nested iteration on squirrel classes with custom _nexti should never be used because if you "break" from the inner loop there is no way for the class to detect that. So, I'd not try to "fix" this - instead people should just not do nested iteration on these classes.

I am a proponent of the idea that the class should get a .toIterator function that will give you a standard array/table to iterate on. E.g. for the weighted container it could be:

function toIterator()
{
   return this.Table;
}

@Suor
Copy link
Contributor Author

Suor commented Sep 4, 2024

I would certainly call it iter() ), and this ._nexti() design is unfortunate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants