Functions for operating on Maps from C

[avivbeeri]

I found myself wanting to pass structured data from Wren to C, but I didn't want to define a foreign class specifically to do that, so I thought having access to Maps from the C slot API made sense.

[mhermier]

This patch is broken/insufficient. It might leak UNDEFINED_VAL and also does not handle/provide enough error handling.

[avivbeeri]

Added something to handle UNDEFINED_VAL.

I suppose the major error case is trying to use an invalid type of object as a map key? Preventing that shouldn't be too difficult, I just need to establish which types are allowed.

[avivbeeri]

Although for what it's worth, I can't see anything in the Wren VM which does error checking like that... so does it even enforce what can be used as a key?

[bjorn]

does it even enforce what can be used as a key?

wren/src/vm/wren_primitive.c

Lines 50 to 59 in 6ab4abe

	bool validateKey(WrenVM* vm, Value arg)
	{
	if (IS_BOOL(arg) || IS_CLASS(arg) || IS_NULL(arg) ||
	IS_NUM(arg) || IS_RANGE(arg) || IS_STRING(arg))
	{
	return true;
	}
	RETURN_ERROR("Key must be a value type.");
	}

[avivbeeri]

Ah, pardon me. I guess I can call validateKey before allowing the value to be set.

[avivbeeri]

Seem that function isn't exposed to wren_vm.h. is it okay to import wren_primitive.h just for this function call?

I don't want to duplicate it, in case the criteria for valid keys changes in the future.

[avivbeeri]

Added in the key-type validation, which hopefully helps cover the potential error scenarios.

[bjorn]

A few nitpicks.

[bjorn]

I think it's a nice addition.

[mhermier]

The fact that all these functions doesn't return an error code at minimum frighten me.

[avivbeeri]

By "all these functions", do you mean (1) the new ones introduced by this PR, or (2) the whole Wren<->C api?

For (1), the functions I've added here are based on the existing API, which is why they have limited fail-modes.

If your concern is (2), there's a high amount of assumption built into the FFI that you know what you are doing, or are performing your own verification, which I believe is done for performance purposes. Trying to address those shortcoming would be out of scope for this PR.

[ruby0x1]

Yes you're correct @avivbeeri, this PR is consistent with the code base as is, and that's reasonable.

[vladimyr]

@avivbeeri I've accidentally duplicated your effort in #760 🤦
After @ruby0x1 pointed me here I went through your code and figured you might want to add few things:

/* wren.h */

// Returns the number of entries in the map stored in [slot].
int wrenGetMapCount(WrenVM* vm, int slot);

/* wren_vm.c */

int wrenGetMapCount(WrenVM* vm, int slot)
{
  validateApiSlot(vm, slot);
  ASSERT(IS_MAP(vm->apiStack[slot]), "Slot must hold a map.");

  ObjMap* map = AS_MAP(vm->apiStack[slot]);
  return map->count;
}

The reason I'm suggesting it because there is already wrenGetListCount so we keep parity between List and Map API.

Aside from that, kudos for great work 🏆

[vladimyr]

You are missing a new line here.

[vladimyr]

@avivbeeri I've rebased your work on top of current master and added wrenGetMapCount. I had to regenerate projects to include new test sources. You can see result here: master...vladimyr:mapFFI

It would be awesome if you could take a look and update this PR accordingly.

[avivbeeri]

I figured this would need some maintenance once 0.3.0 dropped.

I'll get onto it when I have a moment, which will likely be this coming weekend.

[mode777]

Am I missing something or is it not possible to iterate keys of a map with the implemented functionality? This would be important e.g. for serialization.
We at least need a another method void wrenGetMapKey(WrenVM* vm, int mapSlot, int index, int keySlot); right?

[avivbeeri]

At the time I wrote this, you were assumed to know which keys you wanted to extract. The side-step solution is to pass map.keys to your foreign method along with the map you want to act upon.

[ruby0x1]

I had some more functions for iteration coming, I just merged the PR with the foundation :)

[mode777]

@ruby0x1 I'm highly interested in this. Can you give me a hint of how the api will look like. My example from above won't work from what I've learned by now.