[PATCH libxkbcommon] keymap: add xkb_keymap_new_from_memory()

[David Herrmann]

Hi Ran

On Thu, Mar 14, 2013 at 2:14 PM, Ran Benita <ran234 at gmail.com> wrote:
> On Wed, Mar 13, 2013 at 07:28:17PM +0200, Ran Benita wrote:
>> On Mon, Mar 11, 2013 at 12:53:39PM +0100, David Herrmann wrote:
>> > The current API doesn't allow the caller to create keymaps from mmap()'ed
>> > files. The problem is, xkb_keymap_new_from_string() requires a terminating
>> > 0 byte. However, there is no way to guarantee that when using mmap() so a
>> > user currently has to copy the whole file just to get the terminating zero
>> > byte (assuming they cannot use xkb_keymap_new_from_file()).
>> >
>> > This adds a new entry xkb_keymap_new_from_memory() which takes a memory
>> > location and the size in bytes.
>> >
>> > Internally, we depend on yy_scan_{string,byte}() helpers. According to
>> > flex documentation these already copy the input string because they are
>> > wrappers around yy_scan_buffer().
>> > yy_scan_buffer() on the other hand has some insane requirements. The
>> > buffer must be writeable and the last two bytes must be ASCII-NUL. But the
>> > buffer may contain other 0 bytes just fine.
>> >
>> > Because we don't want these constraints in our public API,
>> > xkb_keymap_new_from_memory() needs to create a copy of the input memory.
>> > But it then calls yy_scan_buffer() directly. Hence, we have the same
>> > number of buffer-copies as with *_from_string() but without the
>> > terminating 0 requirement.
>> >
>> > Maybe some day we no longer depend on flex and can have a zero-copy API. A
>> > user could mmap() a file and it would get parsed right from this buffer.
>> > But until then, we shouldn't expose this limitation in the API but instead
>> > provide an API that some day can work with zero-copy.
>> >
>> > Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
>>
>> Patch looks good to me. Too bad we need the extra entry point.
>>
>> As a side note to your last point: replacing flex is pretty easy, in
>> fact I've done it some months ago. I decided against it though because
>> flex files are nice and declarative and there wasn't a good reason to
>> replace it. But I've done a quick rebase now on top of my working branch
>> and your patch:
>> https://github.com/bluetech/libxkbcommon/commits/scanner-wip
>> Maybe it's worth it now.
>
> I've done a V2 and now I'm happy with it, so no more "wip":
> https://github.com/bluetech/libxkbcommon/commits/scanner

The patch looks good. I don't have time to review it properly, sorry.
But at least you can change '\"' to '"'. No backslash needed ;)

Regarding _from_memory(), Daniel said he is fine with it but wants to
rethink the name.

Thanks
David