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

add default field support for object in ARC/ORC #20220

Closed
wants to merge 52 commits into from

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Aug 14, 2022

closes nim-lang/RFCs#126
closes nim-lang/RFCs#48
closes nim-lang/RFCs#233
closes nim-lang/RFCs#252
fixes #19763
fixes #16744
fixes #3608
ref nim-lang/RFCs#437

inspired by #12378

default fields for object

  • result
  • object variant

the addition compared to #12378

  • var
  • omit type for default fields
  • default(Type)
  • new(ref)
  • nested object definition
  • array
  • tuples
  • an object of array etc. in the definition of the object
  • threadvar
  • noinit
  • range
  • fieldVisible issue
  • distinct
  • newFinalize
  • seq(var, result, setLen)
  • docs
  • tests

notes for me only

abstractInst vs {tyGenericInst, tyAlias, tySink}

@ringabout ringabout mentioned this pull request Aug 14, 2022
19 tasks
@ringabout ringabout changed the title add default field support for object add default field support for object in ARC/ORC Aug 15, 2022
@konsumlamm
Copy link
Contributor

Why does the PR say "in ARC/ORC"? Shouldn't this be independent of the GC? Does this mean that the feature doesn't exist for other GCs or what's the deal?

@ringabout
Copy link
Member Author

Why does the PR say "in ARC/ORC"? Shouldn't this be independent of the GC? Does this mean that the feature doesn't exist for other GCs or what's the deal?

I haven't figured out seq support for refc. I will focus on ARC/ORC first.

@zerbina
Copy link
Contributor

zerbina commented Aug 15, 2022

To expand a bit on the issue with seqs @ringabout is referring to: refc and the other non-ARC/ORC memory management strategies use an RTTI-based seq implementation. The default field initialization as implemented here works by rewriting various statements and expressions during the semantic analysis phase.

In the seqs_v2 implementation used for ARC/ORC, initializing the elements to default values on sequence initialization or resize can be (and is) implemented by simply assigning all new elements to default(T) (which then gets rewritten during sem'), but the same isn't possible for the RTTI-based seq implementation, since the static type is erased there.

To @ringabout: a straightforward (though not necessarily the best) approach to make this work for the RTTI-based seqs would be to add a new init: proc field to TNimType, lift the default initialization logic for types into procedures, and adjust the genTypeInfoV1 procedure to also initialize the new TNimType.init field.

# the signature of the lifted init proc:
proc init(p: pointer)

proc setLengthSeqV2(s: PGenericSeq, typ: TNimType, newLen: int): PGenericSeq =
  ...
  for i in oldLen..<newLen:
    typ.base.init(dataPointer(result, elemAlign, elemSize, i))
  ...

# for `newSeq`, `cgen` adjustments would be required

Lifting the init procedures is maybe going to be a bit tricky - I believe introducing a new TTypeAttachedOp (e.g. attachedInit) and then performing the lifting during sempass2 could work.

To not create unused init procedures, only types appearing as the element type for seqs should get their initialization logic lifted.

@ringabout ringabout mentioned this pull request Aug 18, 2022
33 tasks
@Varriount Varriount added the Requires Araq To Merge PR should only be merged by Araq label Aug 19, 2022
@Varriount Varriount requested a review from Araq August 19, 2022 19:41
@Araq
Copy link
Member

Araq commented Aug 24, 2022

It's perfectly fine to combine default field support with ARC/ORC and only enable it for version 2.

@Araq
Copy link
Member

Araq commented Sep 28, 2022

Please resolve conflicts.

And it needs a grammar update as @metagn suggested.

@ringabout ringabout closed this Oct 2, 2022
@ringabout ringabout deleted the test-branch branch October 2, 2022 09:44
@ringabout
Copy link
Member Author

Succeeded by #20480

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment