Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cstring and array[..., char] are treated the same by compiler for hashes.hash() #6350

Closed
eliezedeck opened this issue Sep 8, 2017 · 4 comments

Comments

@eliezedeck
Copy link
Contributor

Using this piece of code:

type
  ClientID* = array[0..5, char]
proc hash*(x: ClientID): Hash =
  return hashes.hash(cast[array[0..5, char]](x))

Whether I cast x to the array or not, I'm getting this error:

Error: ambiguous call; both hashes.hash(x: cstring)[declared in lib/pure/hashes.nim(138, 5)] and hashes.hash(x: openarray[A])[declared in lib/pure/hashes.nim(240, 5)] match for: (array[0..5, char])

I'm not sure if I'm missing something, but it seems simple, yet, I can't find a way to appease the compiler around this. It's a roadblocker.

@mratsim
Copy link
Collaborator

mratsim commented Sep 8, 2017

Had to fight against that as well with regular string.

You have various solutions with examples in my arraymancer library:

Using "not char" - Source:

proc shape[T: not char](s: openarray[T], parent_shape: seq[int] = @[]): seq[int] {.noSideEffect.}=
  ## Helper function to get the shape of nested arrays/sequences
  result = parent_shape & s.len
  when (T is seq|array):
    result = shape(s[0], result)

proc shape(s: string|seq[char], parent_shape: seq[int] = @[]): seq[int] {.noSideEffect.}=
  ## Handle char / string
  if parent_shape == @[]:
    return @[1]
  else: return parent_shape

If you don't use generics, one proc with openarray and another with string (or cstring in your case) seem to work - Source:

proc toTensor*(s:openarray, B: static[Backend]): auto {.noSideEffect.} =
  ## Convert an openarray to a Tensor
  toTensorT(s,B)

proc toTensor*(s:string, B: static[Backend]): auto {.noSideEffect.} =
  ## Convert an openarray to a Tensor
  ##
  ## Handle string specifically (otherwise they are interpreted as openarray[char])
  toTensorT(s,B)

@cooldome
Copy link
Member

IMO, it is more foundation problem. Currently, array[,char] is implicitly castable to cstring and the right solution is to get rid of this implicit cast in the compiler. Nothing hash specific.

For the same reason echo of array[, char] doesn't work

@eliezedeck
Copy link
Contributor Author

@mratsim Thanks for the help. Appreciate the help from fellow Malagasy ;) and glad to see other guys from Dago using Nim.

Actually, I am not aware of this not * feature. I have not yet extensively used all the features that Nim has to provide. But thanks for pointing out that they exist.

@cooldome Indeed, this is not hash specific. I emphasize that this is pretty dangerous, as one could naively just cast to cstring. This becomes dangerous when the content of the variable contains null bytes in between, which is treated as string termination on cstring, but not so in openarray.

@dom96 has suggest in the chat that I could simply cast to cstring. I disagree, and in this particular use case, that would be an absolute disaster. For example, given the byte sequence "Hell\0 there", the hashing value when interpreted as cstring and openarray will be completely different; cstring will only hash 4 bytes, whereas this is actually really 11 bytes, simply because of that \0.

@Araq
Copy link
Member

Araq commented Oct 10, 2017

array[X, char] does not convert to cstring anymore.

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

No branches or pull requests

4 participants