search

brenhinkeller / StaticTools.jl

Enabling StaticCompiler.jl-based compilation of (some) Julia code to standalone native binaries by avoiding GC allocations and llvmcall-ing all the things!
MIT License
167 stars 12 forks source link

Add supertype for the string(s) #6

Closed PallHaraldsson closed 2 years ago

PallHaraldsson commented 2 years ago

Before you merge this, make sure it makes sense.

You can think of this a a question. I'm not sure there's any drawback if you do not use the added capabilities that come with this, and this might be more relevant if you use your strings in a general context (likely not wanted?).

In general you want strings to be subtypes of AbstractString, but it may not apply here. Does it mean you automatically get access to many methods, that will and should work, but not all would work?

It seems to me you're more or less reimplementing (while for any length, not just some fixed lengths): https://github.com/JuliaStrings/InlineStrings.jl

brenhinkeller commented 2 years ago

Ah yeah, so last time I tried this IIRC it broke something because of methods in base that required libjulia. We can at least run the integration tests though and see if anything falls over (though the integration test coverage is probably not great yet!)

brenhinkeller commented 2 years ago

Hmm, so far so good

PallHaraldsson commented 2 years ago

Well it ran on 1.8.0-beta3, and all but: I'm not sure, but likely a false alarm on nightly. I do see you have older GPUCompiler v0.13.14 in CI, likely unrelated.

Test can show bugs, but not absence of bugs. In this case, I'm not sure no failure would for sure say it's ok to merge. Is it a promise some (in fact all) string methods should work; that are not actually being tested?

brenhinkeller commented 2 years ago

Yeah the nightly failure is unrelated -- a lot of things don't work yet on nightly.

I added another integration tests for strings and it's still looking ok on that end (was a bit worried that we'd have to use @inbounds when indexing into strings but looks like not!). I think I must have added specialized methods that are higher in the dispatch hierarchy than whatever was causing problems before.

I'll merge for now and see how it goes! It's easy to undo if anything breaks or anyone complains!