Skip to content

Commit

Permalink
[dce] keep everything if a class extends an extern (closes #6168)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Simn
Simn committed Jun 25, 2017
1 parent eaf0733 commit 23f94bf
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/optimization/dce.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ type dce = {
(* check for @:keepSub metadata, which forces @:keep on child classes *)
let rec super_forces_keep c =
Meta.has Meta.KeepSub c.cl_meta || match c.cl_super with
| Some (csup,_) -> super_forces_keep csup
| Some (csup,_) -> csup.cl_extern || super_forces_keep csup
| _ -> false

let is_std_file dce file =
Expand Down

11 comments on commit 23f94bf

@Simn
Copy link
Member Author

@Simn Simn commented on 23f94bf Jun 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hughsando: This causes a null object reference error on cppia. Any idea what could be going on?

@hughsando
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed by 6046a10

@back2dos
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh. This means if you extend something ubiquitous as EventTarget/EventDispatcher/EventEmitter, your whole class becomes non-DCE-able. Can we have a way to opt out of this at least?

@RealyUniqueName
Copy link
Member

@RealyUniqueName RealyUniqueName commented on 23f94bf Jun 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to auto-keep only fields, which override extern ones.

@Simn
Copy link
Member Author

@Simn Simn commented on 23f94bf Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that would be a bit better indeed. Feel free to change it.

@back2dos
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me clarify my particular issue: this change forcibly disables DCE in a number of cases. Consider this try.haxe snippet:

class Test {
    static function main() {
        trace("Haxe is great!");
    }
}

//@:keepSub //<-- uncomment for the new behavior
extern class EventTarget {}

class Foo extends EventTarget {
    static function bar() {}
}

I cannot come up with a sensible reason for Foo.bar to be in the output. But maybe that's just me and maybe the new behavior is a sensible default. Fine. I still require some way to opt out of it. Pretty please. Thank you ;)

@nadako
Copy link
Member

@nadako nadako commented on 23f94bf Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, keeping static fields doesn't make much sense here. And yes, it would be better to only keep overriden extern methods.

@waneck
Copy link
Member

@waneck waneck commented on 23f94bf Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the whole concept seems to be misled. I think adding @:keepSub on the extern class allows more fine grained control for the user, and making entire projects not dce-able sounds not like a good tradeoff

@Simn
Copy link
Member Author

@Simn Simn commented on 23f94bf Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't realized that it was so common to extend extern classes. Anyway, I already agreed to changing it like @RealyUniqueName suggested, so there's really no reason for further discussion.

@nadako
Copy link
Member

@nadako nadako commented on 23f94bf Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, with this change, a minimal js output now contains HaxeError stuff.

@RealyUniqueName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in 0b3e126

Please sign in to comment.