status-im / nim-chronos

Chronos - An efficient library for asynchronous programming
https://status-im.github.io/nim-chronos/docs/chronos
Apache License 2.0
353 stars 51 forks source link

remove sentinelCallbackImpl #319

Closed bung87 closed 1 year ago

bung87 commented 1 year ago

refs: https://github.com/nim-lang/Nim/pull/20408

const closure not usefull here, so I consider can saftly remove.

arnetheduck commented 1 year ago

not usefull here

the closure is useful / deliberate here: it provides an assert when misused / there's a bug

bung87 commented 1 year ago

I see isSentinel is used, so it may not be mis used internal, does users can access it ?

arnetheduck commented 1 year ago

does users can access it ?

indirectly, when calling functions within the library that use it - directly, no - but that doesn't really matter - what we want to avoid is:

bung87 commented 1 year ago

does users can access it ?

indirectly, when calling functions within the library that use it - directly, no - but that doesn't really matter - what we want to avoid is:

  • global var/let - these cause gcsafe violations
  • new instance every time: unnecessary heap allocations

I understand that, so I cant find easier way to patch this. in the end I change it to nil.

arnetheduck commented 1 year ago

so I cant find easier way to patch this

The capability to make const proc shouldn't be removed from Nim - potentially, it could be limited so that proc's assigned to consts don't do const violations

bung87 commented 1 year ago

I may make it partially support or wait for real const clsoure implementation.

arnetheduck commented 1 year ago

wait for real const clsoure implementation.

this is the way to go - ie Nim shouldn't break existing valid use cases unless there's a strong reason to, and this doesn't strike me as one