Skip to content

ComptimeStringMap improvements - #5520

Closed
data-man wants to merge 4 commits into
ziglang:masterfrom
data-man:ci_string_map
Closed

ComptimeStringMap improvements#5520
data-man wants to merge 4 commits into
ziglang:masterfrom
data-man:ci_string_map

Conversation

@data-man

Copy link
Copy Markdown
Contributor

Useful for HTML entities, tags, color names, etc.

@squeek502

Copy link
Copy Markdown
Member

I'm presuming the usual 'what about unicode' might apply here.

@squeek502

squeek502 commented Jun 29, 2020

Copy link
Copy Markdown
Member

By the way, case insensitive ComptimeStringMaps could be done with the status quo implementation by adding all strings as lowercase or uppercase at comptime and then converting the strings you want to look up to lowercase/uppercase before calling get. That might also be more flexible considering case insensitivity can be weird with Unicode.

Out of curiosity, are there any existing functions that deal with case insensitivity outside of std.ascii?

@data-man

Copy link
Copy Markdown
ContributorAuthor

@squeek502

to look up to lowercase/uppercase before calling get.

It's inefficient.

@squeek502

Copy link
Copy Markdown
Member

True. Maybe ComptimeStringMap should be more like the HashMap implementation and take an eql function? That'd allow for the same flexibility but without the performance hit.

@data-man

Copy link
Copy Markdown
ContributorAuthor

Good idea!
Or can be added:

pubfngetSuperFlexible(str: []constu8, eql: fn (a: var, b: var) bool) ?VpubfnhasSuperFlexible(str: []constu8, eql: fn (a: var, b: var) bool) bool

@data-mandata-man changed the title Add case insensitive functions to ComptimeStringMapComptimeStringMap improvementsJul 4, 2020
@data-man

Copy link
Copy Markdown
ContributorAuthor

Added:
ascii.eql
getWithEqlFn
hasCaseInsensitive
getCaseInsensitive
getMinLen()
getMaxLen()
getItems() because there is no way to get items after creating a map

Comment threadlib/std/ascii.zig Outdated
Comment threadlib/std/comptime_string_map.zig
Comment threadlib/std/comptime_string_map.zig
@data-man

Copy link
Copy Markdown
ContributorAuthor

@squeek502

I'm presuming the usual 'what about unicode' might apply here.

Now you can compare keys any way you want. :)

@data-man

data-man commented Jul 4, 2020

Copy link
Copy Markdown
ContributorAuthor

@squeek502

I'm presuming the usual 'what about unicode' might apply here.

Hmm,

if (kv.key.len!=str.len)
returnnull;

Keys can have different lengths, but can be equal with your comparison function (normalization, casing, etc.).

@squeek502

Copy link
Copy Markdown
Member

Yeah, this implementation is heavily reliant on lengths being a short-circuit for inequality. For more robust/complex equality checks, something like ComptimeHashMap is probably better. See also what I wrote in the PR for ComptimeStringMap:

  • It seems likely that the increased performance of this implementation is not very generalizable and is mostly geared towards the specific case of keywords, since the performance comes mainly from the keys being different lengths, and from the lookup being able to rule out inputs based on length (which also assumes that the get function will be called often with things that can be ruled out, which also seems specific to use cases like keyword lookup).
    • This makes me think this might be better somewhat hidden away but still accessible so that people can use it for their own tokenization implementations. Maybe somewhere in std.zig?

@data-man

Copy link
Copy Markdown
ContributorAuthor

Yeah, this implementation is heavily reliant on lengths being a short-circuit for inequality.

So I think getMinLen() and getMaxLen() will be useful.

@andrewrk

Copy link
Copy Markdown
Member

This has been WIP since June (the freebsd test failures look legitimate), please re-open if you wish to continue working on this.

Sign up for freeto 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.

4 participants

@data-man@squeek502@andrewrk@haze