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

Add keyValueIterator to arrays #891

Merged
merged 2 commits into from
Apr 6, 2020
Merged

Conversation

Aurel300
Copy link
Member

@Aurel300 Aurel300 commented Mar 10, 2020

Closes #871 (and unblocks HaxeFoundation/haxe#7422).

I mostly followed the implementation of iterator for Arrays. Some things that I am not sure about:

  • include/hx/Object.h has a clsIdArrayIterator - should there ba a clsIdArrayKeyValueIterator? I couldn't figure out what these clsId things actually do. For now the KV iterator is also marked as clsIdArrayIterator.
  • src/cppia/Cppia.h, line 116 - I added afKeyValueIterator to the array functions right after the existing afIterator. Is there an external dependency on the exact order of these values?
  • src/cppia/ArrayBuiltin.cpp, line 1697 - is the conversion to destType there just "convert to object"?
  • include/Array.h and src/Array.cpp - I added a hx::ArrayKeyValueIterator class to mirror the hx::ArrayIterator one. I could not define the Dynamic next() method in the header file because I cannot include Anon.h in Array.h (cyclic dependency). The implementation of is in src/Array.cpp.

The CI failure seems to be related to the last point. I guess src/Array.cpp (or its object file) is not being linked wherever things from src/Array.h are used?

@Aurel300 Aurel300 requested a review from hughsando March 10, 2020 13:48
@hughsando
Copy link
Member

The clsId is for "Std.is" - since you can't ask this for iterators (?), it does not matter.

The order of the ArrayFunc enum needs to match the gArrayFuncNames and gArrayArgCount order, but these are not saved to disk anywhere so just need to be consistent for the single compile.

It is quite possible the destType will be String (eg, "trace"). With some Dynamic stuff, it might end up being Int too, but the result would not be important here, since it's undefined behaviour.

I can't see the error, but I think it's because the c++ code needs to be able to "see" the definition if its templated. The best place to put this is in "include/hx/Operators.h" which comes after everything else is included.

@hughsando
Copy link
Member

but also, good work.

@nadako
Copy link
Member

nadako commented Apr 5, 2020

@Aurel300 let's do this for 4.1 :)

@hughsando hughsando merged commit d32d80a into HaxeFoundation:master Apr 6, 2020
@Aurel300 Aurel300 deleted the issue/871 branch April 6, 2020 15:29
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.

Array.keyValueIterator() implementation
3 participants