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

[TODO] VM: allow const ref objects #13082

Closed
wants to merge 2 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jan 9, 2020

this PR enables const ref objects.
It seems to work, and makes it usable to use more code at CT.
The pre-existing caveat (when the ref object points to a case object) is a separate, pre-existing issue (see #13081) that will get fixed eventually, but shouldn't block this; the problem is the case-object not the ref object

example use case1: with ref object

type Config = ref object
  paths: seq[string]
  name: string

const j = parseFile("cfgFile.json").to(Config)
# now you can use j.paths, j.name etc at runtime; no caveats

example use case2: even useful with ref case objects

import std/json
const j = parseFile("cfgFile.json")
const paths = j["paths"].to(seq[string]) # extract const field to avoid caveat mentioned in https://github.com/nim-lang/Nim/issues/13081
const name = j["name"].to(string)
# now you can use paths, name at runtime

example use case3: const TableRef

from here: #13430 (comment)

import tables
const t = {1:"foo", 2:"bar"}.newTable
doAssert $t == """{1: "foo", 2: "bar"}"""
doAssert t is TableRef
doAssert t[1] == "foo"
static: doAssert t[1] == "foo"

@Araq
Copy link
Member

Araq commented Jan 9, 2020

Er what? The C codegen does not support that and it bites the GCs.

@Araq
Copy link
Member

Araq commented Jan 23, 2020

No feedback, closing for now as I noticed Timothee won't mind.

@Araq Araq closed this Jan 23, 2020
@timotheecour
Copy link
Member Author

/cc @Araq still working on this (had a bunch of other PRs in the meantime) I had written additional tests which I didn't push yet, it still seems to all just work except for the above mentioned case objects #13081 which is a separate issue; I can hide this behind an experimental flag if needed in the meantime until #13081 is addressed , but it does solve practical problems and makes nim less fragmented bw VM vs RT; I'm not sure what you mean by bites the GCs , do you have an example where this would fail (that isn't #13081) ?

@timotheecour timotheecour changed the title VM: allow const ref objects [TODO] VM: allow const ref objects Jan 23, 2020
@Araq
Copy link
Member

Araq commented Jan 23, 2020

For example:

const x = ConfigRef(...)
var y = x # GC's write barrier assumes this has a PCell header, but it doesn't!
#--> crash.

@timotheecour
Copy link
Member Author

timotheecour commented Jan 30, 2020

@Araq I'm not seeing this; if I use:

import compiler/options
const x = ConfigRef()

it simply gives a CT error invalid type for const: ConfigRef and after investigation that's because ConfigRef contains a object of RootObj field, which is out of scope for this PR; likewise with simply:

  type
    Bar = object of RootObj
  const j = Bar()

however if I don't use a object of RootObj object, I dont' get a crash:

  type Foo = ref object
    x1: int
    x2: string
    x3: seq[string]
  proc main() =
    let n = 1_000000
    for i in 0..<n:
      const j2 = Foo(x1: 12, x2: "asdf", x3: @["foo", "bar"])
      var j2b = j2
      echo j2b[]
  main()

=> no crash

(and even if it did crash, the PR could be updated to forbid var x = val where val is a const ref, if needed)

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.

2 participants