search

unicode-rs / unicode-xid

Other
49 stars 28 forks source link

V0.2.0 #12

Closed matklad closed 5 years ago

matklad commented 5 years ago

wait a sec, want to pull other minor changes here

lnicola commented 5 years ago

Should this be 0.2.0?

Manishearth commented 5 years ago

Yeah, make this 2.0

matklad commented 5 years ago

@Manishearth hm, than let's make 1.0.0 then?

Manishearth commented 5 years ago

I'd rather not for now.

Manishearth commented 5 years ago

I'm okay with a 1.0.0, I just don't have the bandwidth to do the reviews for it.

matklad commented 5 years ago

Actually, let me write this down in the long form:

I think upgrading unicode version for this crate should be considered a minor (that is, non-semver breaking) change. This is nice, because all depndencies will support newer unicode versions "for free".

So, it makes sense to release this as 0.1.1 (remember, if major is 0, then patch means minor).

OTOH, we really should do an 1.0.0 for this crate, and this is a good occasion for this! 1.0.0 shouldn't be disrupting, because this is not an interface crate. Making 1.0.0 makes sense, because literally half of the ecosystem depends on this crate via proc_macro2.

If we want to treat unicode upgrades as semver breaking, than yes, it makes sense to do 0.2.0.

However, I don't have a strong opinion here, so I'll just optimistically change this to 0.2.0. I'll also be happy to help with maintenance :)

Manishearth commented 5 years ago

Travis fails on some tests.

Yeah, I think in general we've made unicode updates breaking changes. But perhaps we shouldn't once we have 1.0.0.

Note that unicode updates often come with algorithm changes (this one doesn't).

Totally down for a 1.0.0, someone else needs to do the reviewing work.

Manishearth commented 5 years ago

For some reason this implementation is 10x as slow as the stdlib, btw

matklad commented 5 years ago

@Manishearth they use different algorithms: this crate uses binary search, stdlib uses a trie.

Totally down for a 1.0.0, someone else needs to do the reviewing work.

I think if we decide to move compiler over to this crate instead of libcore, t-compiler will be in charge of maintaining this (and will probably do 1.0)

Manishearth commented 5 years ago

Sounds good.

Manishearth commented 5 years ago

Should we also be using a trie?

matklad commented 5 years ago

That's code-size/time trade off iiuc. I think binary search is a better default, because ASCII typically has a different fast-path, so we are not that concerned with performance.

The "proper" solution here is to do both, via a feature flag. Should be relatively easy to do if we switch from python to https://github.com/BurntSushi/ucd-generate

lnicola commented 5 years ago

Perhaps a fast-path for ASCII characters would recover any performance loss here.

because ASCII typically has a different fast-path

Ah...