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

WIP | =sink is now =move #11248

Closed
wants to merge 25 commits into from
Closed

Conversation

Clyybber
Copy link
Contributor

@Clyybber Clyybber commented May 14, 2019

... which makes self-assignments work correctly.
Refs #11217

compiler/liftdestructors.nim Outdated Show resolved Hide resolved
compiler/liftdestructors.nim Outdated Show resolved Hide resolved
compiler/semstmts.nim Outdated Show resolved Hide resolved
lib/system.nim Outdated Show resolved Hide resolved
@Clyybber Clyybber changed the title =sink is now =move WIP | =sink is now =move May 16, 2019
@Araq
Copy link
Member

Araq commented May 29, 2019

Note: I'm taking over, I really want this in 0.20.

@Clyybber Clyybber force-pushed the unifysinknmove branch 5 times, most recently from 797fb24 to 605a266 Compare June 7, 2019 15:40
@Clyybber
Copy link
Contributor Author

Clyybber commented Jun 7, 2019

TODO:

  • handle explicit moves better??
  • const temporaries
  • adapt tests

EDIT: I think this elides destructors too agressively, investigating..

@Araq
Copy link
Member

Araq commented Jun 8, 2019

You need to patch the C codegen so that tySink can also be "pass by hidden pointer". Also this section

  if owner.kind in {skProc, skFunc, skMethod, skIterator, skConverter}:
    let params = owner.typ.n
    for i in 1 ..< params.len:
      let param = params[i].sym
      if isSinkTypeForParam(param.typ) and hasDestructor(param.typ.skipTypes({tySink})):
        c.destroys.add genDestroy(c, param.typ.skipTypes({tyGenericInst, tyAlias, tySink}), params[i])

needs to be removed, the caller is responsible for cleaning up what was passed as a sink parameter.

@Clyybber Clyybber force-pushed the unifysinknmove branch 2 times, most recently from d510937 to 7a9a1d4 Compare June 16, 2019 14:57
@Clyybber Clyybber force-pushed the unifysinknmove branch 4 times, most recently from 01525b2 to 582d40a Compare June 27, 2019 11:04
@Clyybber Clyybber force-pushed the unifysinknmove branch 2 times, most recently from c62c052 to 92454d5 Compare July 7, 2019 21:39
@Clyybber Clyybber force-pushed the unifysinknmove branch 3 times, most recently from b5cdd4d to a70c846 Compare July 13, 2019 19:14
@Clyybber
Copy link
Contributor Author

Clyybber commented Aug 1, 2019

TODO:

  • Make closures work
  • Fix obj and tuple constructors of the backends: (affected test: tgcdestructors.nim)
    This is a bit tricky, see last commit for a dirty leaking unmergeable workaround.
    @Araq The problem is this:
type
  Obj* = object
    f*: seq[int]

var o = Obj(f: @[1, 2, 3])

will be transformed to:

eq__qdqSn882Av3rQxYVfnQyMg((&colontmpD_), TM_mDIVXahtYVExBX9bJLnfz0Q_76); //Copy, because const seq
T2_.f = colontmpD_;
eqmove__C1DzWNOtypwtWid2xBvXLg((&o), (&T2_));
//...
eqdestroy__39bIYiz25XeqkqfTm4wBXNg((&colontmpD_));
eqdestroy__9aQE9amztaLIv7YEpj9agHaSg((&o));

which will cause a double free segfault, since colontmpD_ is shallow copied to the field of T2_ / o.

@Clyybber Clyybber force-pushed the unifysinknmove branch 2 times, most recently from 6634013 to fd3f7df Compare August 7, 2019 11:55
@Clyybber
Copy link
Contributor Author

This turned out to solve a problem that doesn't exist, and also hurt performance by 5% on avg.

@Clyybber Clyybber closed this Aug 11, 2019
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.

None yet

2 participants