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

*explode return array size. #700

Closed
wants to merge 5 commits into from
Closed

*explode return array size. #700

wants to merge 5 commits into from

Conversation

Emistry
Copy link
Member

@Emistry Emistry commented Sep 6, 2015

@dastgirp
Copy link
Member

This would fail.
Consider I am using
explode .@str$[5],"a,b",",";
The size would return 7
But I would have expected the size to be 2

Also, if one part is returning value,
All other fail condition should return 0 too.

@dastgirp dastgirp added component:core Affecting the Hercules core (i.e. not the game mechanics directly) codereview:needsedits Some edits have been requested before the pull request can be accepted labels Sep 20, 2015
@Emistry
Copy link
Member Author

Emistry commented Sep 20, 2015

I believe it should stay as 7 ..... if it returned 2, it could lead to false array size value if the users have assigned value at the first 5 index of the array.
even though the array could be stored with "empty" string, it's still count as 1.

if it return 2, the script might potentially overwritten any value in the array if the users used the returned value next.

@dastgirp
Copy link
Member

Explode must return how many indexes it has written to according to me...
Like I have custom script, in which I want to use explode from end of my array, so in that case,I would have assumed the return value is new indexes that's been added. And not the size...

Maybe let someone else chime in to discuss what's better
@MishimaHaruna

@Emistry
Copy link
Member Author

Emistry commented Sep 20, 2015

it's the same case.

setarray .@array,0,0,0,0,0,1,2;   // array size = 7.  not 2...
setarray .@array,0,0,0,1,2,0,0;  // array size = 5, not 7, not 2 ...

the purpose of this changes was to reduce the usage of getarraysize() after used explode().

@MishimaHaruna
Copy link
Member

In my opinion, the main problem here is that it's like getarraysize(), but not quite the same.
The value returned in this pull request isn't the size of the (non-sparse) array, like getarraysize returns, but rather, the index of the last element written + 1. This is especially confusing in case you're adding/overwriting elements in an array that wasn't previously empty:

.@array$[10] = "foo";
debugmes getarraysize(.@array$); // 11
.@str$ = "a,b,c,d";
debugmes explode(.@array$[2], .@str$, ","); // 6
debugmes getarraysize(.@array$); // 11 != 6

I think this is misleading. I'd rather return the number of elements written, like Dastgir suggests. That way, if you need the index of the last element written, you can still retrieve it, since you know the index of the first element written:

.@array$[10] = "foo";
.@str$ = "a,b,c,d";
.@n = explode(.@array$[2], .@str$, ",");
debugmes .@n + " " + (2+.@n) + " " + getarraysize(.@array$); // 4 6 11 (elements written, index of the first element after the last written, size of the array)

On a side note, I just noticed the explode function leaks memory. The aMalloc call needs to be moved to after the various checks (just above the while). I'll fix that after this pull request is edited and merged (unless someone wants to do it before)

@MishimaHaruna MishimaHaruna added type:enhancement Issue describes an enhancement or feature that should be implemented status:inprogress Issue is being worked on / the pull request is still a WIP labels Sep 25, 2015
@MishimaHaruna
Copy link
Member

Taking ownership of this. I'll handle the problems mentioned

@MishimaHaruna MishimaHaruna self-assigned this Dec 20, 2015
@Emistry Emistry closed this in 6a0093b Dec 20, 2015
@MishimaHaruna MishimaHaruna removed the status:inprogress Issue is being worked on / the pull request is still a WIP label Dec 20, 2015
@MishimaHaruna
Copy link
Member

Merged as 6a0093b, thank you

@MishimaHaruna MishimaHaruna removed the codereview:needsedits Some edits have been requested before the pull request can be accepted label Dec 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:core Affecting the Hercules core (i.e. not the game mechanics directly) type:enhancement Issue describes an enhancement or feature that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants