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 data type mapping between FFI and Lua values #1059

Merged
merged 5 commits into from Mar 6, 2017

Conversation

wingo
Copy link
Contributor

@wingo wingo commented Nov 3, 2016

From the README:

Ever been annoyed that you can't create a hash table where the keys are
FFI values, like raw IPv4 addresses, but the values are Lua objects?
Well of course you can key a normal Lua table by any Lua value, but the
key is looked up by identity and not by value, which is rarely what you
want. foo[lib.protocol.ipv4:pton('1.2.3.4')] will not be the same as
foo[lib.protocol.ipv4:pton('1.2.3.4')], as the pton call produces a
fresh value every time. What you usually want with FFI-keyed tables is
to be able to look up the entry by value, not by identity.

Well never fear, cltable is here. A cltable is a data type that
associates FFI keys with any old Lua value. When you look up a key in a
cltable, the key is matched by-value.

From the README:

  Ever been annoyed that you can't create a hash table where the keys are
  FFI values, like raw IPv4 addresses, but the values are Lua objects?
  Well of course you can key a normal Lua table by any Lua value, but the
  key is looked up by identity and not by value, which is rarely what you
  want.  `foo[lib.protocol.ipv4:pton('1.2.3.4')]` will not be the same as
  `foo[lib.protocol.ipv4:pton('1.2.3.4')]`, as the `pton` call produces a
  fresh value every time.  What you usually want with FFI-keyed tables is
  to be able to look up the entry by value, not by identity.

  Well never fear, *cltable* is here.  A cltable is a data type that
  associates FFI keys with any old Lua value.  When you look up a key in a
  cltable, the key is matched by-value.
@wingo
Copy link
Contributor Author

wingo commented Nov 4, 2016

seems i forgot to add Igalia@580d8c7 to this branch

wingo and others added 2 commits November 4, 2016 17:53
@wingo
Copy link
Contributor Author

wingo commented Nov 7, 2016

@eugeneia would you perhaps mind having a look at this one? No rush though, I just wanted to get it on board one of the upstream trains :)

@eugeneia eugeneia self-assigned this Nov 7, 2016
Copy link
Member

@eugeneia eugeneia left a comment

Choose a reason for hiding this comment

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

Generally looking shiny! See my comments for suggestions.

cltable.values[entry.value] = value
if value ~= nil then
cltable.keys:remove_ptr(entry)
-- FIXME: Leaking the slot in the values array.
Copy link
Member

Choose a reason for hiding this comment

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

This!

Copy link
Member

Choose a reason for hiding this comment

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

Wait, shouldn’t this be value == nil, as in “if value is nil, delete the entry and set the corresponding key in the backing table to nil, too”.

Copy link
Member

Choose a reason for hiding this comment

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

On second look the latter is performed unconditionally.

end
end

function selftest()
Copy link
Member

Choose a reason for hiding this comment

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

As per the above I would suggest to test deletion here.

@@ -91,8 +91,9 @@ end
-- FIXME: For now the value_type option is required, but in the future
-- we should allow for a nil value type to create a set instead of a
-- map.
local required_params = set('key_type', 'value_type', 'hash_fn')
local required_params = set('key_type', 'value_type')
Copy link
Member

Choose a reason for hiding this comment

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

In theory you could use the new core.lib.parse function for this.

@wingo
Copy link
Contributor Author

wingo commented Nov 8, 2016

Fixed nit, good catch :)

Also it turns out the slot is not leaked. The next slot will be allocated at #values, which may be any index which precedes a nil and in practice seems to be the first one. So the normal case of adding and removing shouldn't grow the values array. Additionally if it did grow and get holey, that would be LuaJIT's problem to switch to a sparse array representation and not a packed array.

Re ctable and parameters it is a good idea for later :)

eugeneia added a commit that referenced this pull request Nov 8, 2016
@eugeneia eugeneia added the merged label Nov 8, 2016
@eugeneia eugeneia merged commit d64e70d into snabbco:master Mar 6, 2017
dpino pushed a commit to dpino/snabb that referenced this pull request May 4, 2018
Make "offset" arg to lseek a signed integer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants