Skip to content

Conversation

@guzba
Copy link
Collaborator

@guzba guzba commented Jun 9, 2022

this enables windy to not need to copyMem before decoding an image here: https://github.com/treeform/windy/blob/master/src/windy/platforms/macos/platform.nim#L962

@SolitudeSF
Copy link

why not just change signature to openArray[char]?

@treeform treeform merged commit ab63f2e into treeform:master Jun 10, 2022
@guzba
Copy link
Collaborator Author

guzba commented Jun 10, 2022

@SolitudeSF I could do openArray, but that has its own issues.

  1. The type system complaining about char vs uint8. Would decodePng need to take openArray[char | uint8 | int8 | byte]? This would also apply to the flatty read* functions we use. I do not care what someone wants to call their bytes but the type system definitely does and it's annoying especially in the Nim VM where if I recall correctly I can't just cast.

  2. openArray isn't an object than can be put anywhere, eg it cannot be added as value on an object. This means I can't have a DecoderState or similar. Instead, I'd have to pass the openArray alongside it in every function instead of just this: https://github.com/treeform/pixie/blob/master/src/pixie/fileformats/jpeg.nim#L1041

In the PNG case openArray would just fine as written, however ->

  1. We are working with OS stuff as well as Nim stuff, and the OS gives pointers and lengths: https://github.com/treeform/windy/blob/master/src/windy/platforms/macos/platform.nim#L962

I could wrap that in an openArray, but what is the point?

I get openArray is useful for seq | array procs, but because I can't treat it like a "thing" it ends up no being worth using in many of my cases thus far. Why wrap a ptr + len from the OS in something new for no reason? Same for opening a memory-mapped Zip Archive. If have a ptr + len already, why wrap it to decode a PNG from inside the archive (and then have to unwrap it or make a new one for ptr+len in the larger buffer)?

@guzba
Copy link
Collaborator Author

guzba commented Jun 10, 2022

Just adding that I have used openArray over here https://github.com/guzba/zippy/blob/master/src/zippy/inflate.nim#L17, I just find it doesn't fit well all the time.

@SolitudeSF
Copy link

im not advising for it as a single unifying api, im saying that its a straight upgrade from just string.

@SolitudeSF
Copy link

SolitudeSF commented Jun 10, 2022

i would even go as far to say that string without var as procedure parameter should never be used. ofc, in case of decoding image data, its rare to not have it as a string or pointer+len already, but when situation arises where someone has openArray for decoding, the string overload does nothing and they have to resort to pointers, when perfecrtly capable solution already exists.

@guzba
Copy link
Collaborator Author

guzba commented Jun 10, 2022

Ah, I do see your point for string vs openArray[char] when not a var. I don't disagree, however I do think openArray[char] kind of hides how an API would get used 99% of the time (for a string). Anyone other than fairly Nim-invested people might just think there isn't a string API perhaps? It also isn't searchable in docs or Google as much, it just kind of makes it blurry.

That shouldn't be a reason not to make things better but it is a concern I'd need to overcome myself.

A small but more practical difference that is a sort of gotcha I've run into before:

This is easy with a string:

decodePng(data.cstring, data.len)

Whereas you need to be careful with openArray:

if data.len > 0:
   decodePng(data[0].unsafeAddr, data.len)
else:
  # fail

All of our repos will evolve so who knows, maybe we'll go all openArray before long, it's not like we can't ever switch to it. I don't currently see it as the most pressing issue though.

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

3 participants