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

$ works for ref|ptr|pointer for all targets (c,cpp,js,vm) + other bugfixes #14043

Closed
wants to merge 2 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 21, 2020

  • tests/system/tostring.nim now enabled for js target after doing the minimal modifications to support it; some failing tests indicate pre-existing nim js bugs that should be fixed
  • fixes a bug for cast in VM which was causing a crash
  • $ now works for ref|ptr|pointer for all targets (c,cpp,js,vm),showing the address, which is useful, eg for determining whether 2 pointers/ref objects are the same (eg inside a log)

$ for ref|ptr|pointer

for nim js it returns a unique object id, eg @123
for all other targets it returns hex address, eg 0x10d766608

the addresses should furthermore be deterministic across re-runs if you disable ASLR, eg by passing --passl:-Wl,-no_pie (see example 2)

see tests in tests/system/tostring.nim

example 1

# D20200421T000023
type
  TFoo = object
    x1: int
    x2: pointer
    x3: seq[Foo]
    x4: ptr TFoo
  Foo = ref TFoo

template fun() =
  var z = 0
  var a = Foo(x1: 1, x2: z.addr)
  a.x3 = @[nil, a, Foo()]
  a.x4 = a[].addr
  echo a[]

fun()
static: fun()
(x1: 1, x2: 0x0, x3: @[nil, 0x10afe4dc8, 0x10b03b808], x4: 0x10afe4dc8)
Hint: ... [SuccessX]
(x1: 1, x2: 0x108e80580, x3: @[nil, 0x108ed2050, 0x108ed2080], x4: 0x108ed2050)

instead of what's printed before this PR:

(x1: 1, x2: ..., x3: ..., x4: ...)
Hint: ... [SuccessX]
(x1: 1, x2: ..., x3: ..., x4: ...)

which doesn't tell you anything useful about reference semantics (eg which pointers are the same)

example 2

git clone https://github.com/timotheecour/vitanim && cd vitanim
nim c -d:case2 --passl:-Wl,-no_pie testcases/t10595.nim
testcases/t10595
testcases/t10595 # shows same addresses

note

  • before PR $ doesn't compile for ref,ptr,pointer which is not a very useful behavior, in particular for debugging (or, inside an object/tuple, would show as ... which isn't particularly useful). The current behavior gives you no easy way to check which ref/ptr objects are identical. Showing the addresses allows exactly that, is safe, and also gives a hint as to whether an address is on the stack, heap, or data segment based on its range.

  • refactoring and yak shaving. #13687 attempted to make $ recurse inside ref objects however I've explained here Every value should be printable with echo RFCs#203 (comment) why this is a bad idea as a default behavior

  • a more sophisticated and customizable pretty-printing (std/prettyprint as I suggested in Every value should be printable with echo RFCs#203 (comment)) doesn't remove the need for $ to work on pointer-like types, since importing std/prettyprint won't work for some modules due to cyclic imports

@timotheecour timotheecour marked this pull request as ready for review April 21, 2020 11:01
@Araq
Copy link
Member

Araq commented Apr 21, 2020

But I recently rejected any further extensions to $. Also please don't conflate bugfixes with feature additions.

@stale
Copy link

stale bot commented Apr 21, 2021

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Apr 21, 2021
@timotheecour
Copy link
Member Author

TODO: move $(ref) to a module other than system

@stale stale bot removed the stale Staled PR/issues; remove the label after fixing them label Apr 21, 2021
@timotheecour timotheecour marked this pull request as draft April 21, 2021 22:24
@stale
Copy link

stale bot commented Apr 24, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Apr 24, 2022
@stale stale bot closed this May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants