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

{.cast(Performance, ProveInit, UnsafeSetLen, ...).} to disable hint/warning in a scope #317

Open
timotheecour opened this issue Jan 16, 2021 · 1 comment

Comments

@timotheecour
Copy link
Member

timotheecour commented Jan 16, 2021

proposal

extend {.cast.} to more use cases
right now it allows {.cast(noSideEffect).}, {.cast(gcsafe).} eg as follows:

when true:
  func bar() =
    var a = 1
    {.cast(noSideEffect).}:
      var b = 2
      echo "ok1"
    {.cast(noSideEffect).}:
      echo (a, b)
    debugEcho (a, b)
  bar()

this proposal extends this) concept to these:

{.cast(Performance).}
{.cast(UnsafeSetLen).}
{.cast(ProveInit).}
...

to disable hint/warning/error in a scope

example 1

{.cast(Performance).}
add(result, a)
{.pop.}

example 2

would give a good solution to the classic NotNil initialization problem, refs nim-lang/Nim#13808 (comment)

when true:
  type A = object
    w: range[2..8]
  proc fn(n: int): seq[A] =
    # for i in 0..<n: result.add A(w: 2) # no ProveInit warning but inefficient, refs https://github.com/nim-lang/Nim/pull/13808#discussion_r401327323
    {.cast(ProveInit).}:
      result = newSeq[A](n)
    for i in 0..<n: result[i] = A(w: 2)

  proc fn2(n: int): seq[A] =
    result = newSeq[A](n) # would correctly trigger `ProveInit` here unless silenced by another {.cast(ProveInit).}:

example 3

similar to Example 2 but for UnsafeSetLen; user code should be able to locally silence
Warning: setLen can potentially expand the sequence, but the element type 'A' doesn't have a valid default value [UnsafeSetLen]

when true:
  type A = object
    a0: range[2..8]
  proc fn(n: int): seq[A] =
    {.cast(UnsafeSetLen).}:
      result.setLen n
    for ai in mitems(result): ai.w = 2

example 4

would fix nim-lang/Nim#3608 or at least provide a suitable workaround

when true:
  type A = ref object
    w: range[2..6]
  proc createABC(): A =
    # result = abc(w: 2) # works but may be undesirable
    {.cast(ProveInit).}:
      new(result)
    result.w = 2

likewise, would provide a suitable workaround for nim-lang/Nim#16735

example 5

would provide a suitable workaround for nim-lang/Nim#9466 (issue is getting closed with nim-lang/Nim#17538 as the error disappeared but the warning is still there)

alternatives considered

{.push hint[Performance]:off.}
code
{.pop.}

# or:
{.hint[Performance]:off.}:
  code

# or:
{.hint[Performance]:off.}
code
{.hint[Performance]:on.} # bad: would set to on even if wasn't set to on before setting to off

these have following downsides:

  • has global effect
  • push/pop is not lexically scoped and easy to get wrong, especially given fact that pop is IIRC optional
  • it's subject to the distinction of foreign package vs main package notes
  • it doesn't actually work depending on the hint/warning

eg, there's no way AFAIK to make it work with current pragmas ({.hint[Performance]:off.} or push etc)

when true:
  type A = object
    w: range[2..8]
  proc fn(n: int): seq[A] =
    # for i in 0..<n: result.add A(w: 2) # no ProveInit warning but inefficient, refs https://github.com/nim-lang/Nim/pull/13808#discussion_r401327323
    # {.push warning[ProveInit]:off.}
    {.warning[ProveInit]:off.}:
      result = newSeq[A](n)
      for i in 0..<n: result[i] = A(w: 2)
    # {.warning[ProveInit]:off.}
    # {.pop.}

  proc fn2(n: int): seq[A] =
    # this should give `ProveInit` warning but does not
    result = newSeq[A](n)
@Araq
Copy link
Member

Araq commented Jan 16, 2021

Good idea but cast(Performance) is not unsafe while the others are. Maybe we need a different pragma name instead. However, the performance hint can also be avoided by an explicit copy call. (Was planned, not sure if it has been added already.)

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

2 participants