> Throughout the history of SWCs, I can "know" that if I change the base > class hierarchy in a way that doesn't change the API surface of that base > class, that I don't have to re-compile the downstream SWCs. With the depth > approach, we are changing those rules, so that's why I am not in favor of > assigning numbers related to depth. It isn't about mixing older swcs with > newer ones. It is simply about expectations on what changes require > re-compiles in downstream SWCs. > > Right, and the change I suggested for swcs would, I believe, fix that, but adds more 'noise' to the names for no user benefit.
> I think we are in favor of the shortest possible suffix that distinguishes > a private variable named "foo" in class A from private variables of the > same name in the base classes. For me, when debugging, it doesn't matter > if I know what class I'm debugging, I still want to know of the "foo" > variable that has the value "bar" is from the current class, the base class > or some class further up the chain and numbers won't help me. > Thanks - I 'get' it now. I was not thinking about the private values in the inspector from the ancestors, because when debugging normal swf, you would never see them. That makes sense to me know :), > So, having an alias table seems like a reasonable solution. It won't be > as short as "P1", but I think it will make debugging easier. You are not > obligated to implement the alias table for other package renaming. Someone > else can do that. > > My 2 cents, > -Alex > > On 11/13/18, 11:36 AM, "Greg Dove" <[email protected]> wrote: > > Actually, I hadn't considered the issue you just brought up about the > inheritance chain changing in a separately compiled project. I would > say > that is a valid concern and probably means that "depth" approach is > not the > right one. > > I think the swc issue is a specific type of issue with the 'depth' > approach > that may be part of a more general issue with swcs that depend on other > swcs. I probably wouldn't try swapping out one of the framework swcs > built > from 6 months ago, although some things might work. > Depth based naming could still be used, but for swc targets, it would > probably need another aspect to the naming that was a short random > sequence, or even related to the time/date (at date level, not more > granular than that) for when the swc was published, maybe 3 letters > after > the P and before the number. I think that would be reasonably safe > ('days > since Royale 1.0 release' could be a good way to encode a 3 char > sequence > like this in the future). So I think the idea could still work, but it > is > not quite as simple as I was aiming for, because of swcs. > > The compiler is already not as fast as we hoped and we need to be > careful > about adding loops that are expensive. > > Yeah, I was going to check this. I'm not looping to check this myself, > I'm > just using the existing support built into ClassDefinition. I did not > look > inside the method I used but I would expect/hope it caches the results > internally, because it is named 'resolve' which makes me think it > could be > expensive. But I will check. My hope is that it has already done that > hard > work at some other stage, and we can just leverage that for something > else > in this case. > > > One option is to introduce an "alias" table in the compiler. You can > give > it package names and a short name to substitute for it. So if we say > that > org.apache.royale.core will be replaced by "oarc" then the decorated > name > for foo would be _foo_oarcBaseClass and _foo_oarcMyClass > > The package naming is to me a separate issue. I am not sure whether > there > are any simpler options that could be used within each file, like a > shorter > local external variable alias after the initial definitions that would > be > omitted by GCL from the release mode (I did not check GCL for whether > this > is possible, but if it is it could perhaps help with this a bit). > But I do also think a lot of these issues would go away once we get to > es6 > output, because that would be much closer to the original > actionscript. I > would be keen to look at es6 output as a separate emitter in the coming > months. The hard work has already been done with what we have, so > maybe it > would not take too long. But for now I am on more concerned with > getting to > 1.0 as-is. > > In terms of private naming approaches > the short package with class suffix is interesting, but for me the > issue is > not about knowing which class I am in. That's always on-screen in the > tab > just above the breakpoint somewhere. > My preference was more about removing the noise of the > 'non-variable-name' > content in debug mode, and making debugging with this on (which I > think I > would prefer by default in the general case) more palatable. > I don't like the $P stuff either, but we must use something, and I > thought > depth was more related to the nature of the problem than the fully > qualified name. > The example you provide above is better, but I still think it takes > more > effort to identify the only part I am interested in that corresponds > to the > original actionscript. > But I am sure everyone has different views and experiences here. There > is > no one 'right' answer that will suit everyone. > > There may of course be some side-benefits for having the depth level > 'in-your-face' for the developer ;) > It could be a wake-up call for people who see really high numbers. Or > at > least improve awareness of 'composition over inheritance'. > > Anyway - I would be still keen to pursue this, checking to make sure > there > is no discernible performance hit, and looking at the swc variations to > avoid potential for conflict there, but I don't want to do anything > that > others won't find useful too, so I won't continue unless there is > agreement. So Alex, Harbs, and anyone else reading this - give me a +1 > or > -1 or further comments and I'll decide based on those. In any case, I > need > to hold off this and any other compiler work (localId changes) until > next > week because of other priorities and looming deadlines. > > thanks, > Greg > > > > > > > > On Tue, Nov 13, 2018 at 2:03 PM Alex Harui <[email protected]> > wrote: > > > Actually, I hadn't considered the issue you just brought up about the > > inheritance chain changing in a separately compiled project. I > would say > > that is a valid concern and probably means that "depth" approach is > not the > > right one. I was mostly concerned about compiler performance and > whether > > the compiler has efficient access to the depth if the base classes > are in > > separately compiled project. The compiler is already not as fast as > we > > hoped and we need to be careful about adding loops that are > expensive. > > > > FWIW, a while ago there was another discussion about output size and > the > > number of long strings related to package names. One option is to > > introduce an "alias" table in the compiler. You can give it package > names > > and a short name to substitute for it. So if we say that > > org.apache.royale.core will be replaced by "oarc" then the decorated > name > > for foo would be _foo_oarcBaseClass and _foo_oarcMyClass > > > > I think that's better than $P1 and $P2 because it does help to know > which > > class's private variables you are looking at without knowing the > chain of > > baseclasses. And elsewhere the download size for Royale apps will > shrink. > > > > Thoughts? > > -Alex > > > > On 11/12/18, 10:41 AM, "Greg Dove" <[email protected]> wrote: > > > > btw I did not get to this last week like I hoped. But I did look > at > > this a > > bit yesterday and I have a local branch that does this for the > original > > example at the beginning of this thread (which I used as my one > 'real' > > example for local testing): > > > > org.apache.royale.core.BaseClass = function() { > > org.apache.royale.utils.Language.trace(this._foo_$P1); > > }; > > org.apache.royale.core.BaseClass.prototype._foo_$P1 = "foo,"; > > > > and > > org.apache.royale.core.MyClass = function() { > > org.apache.royale.core.MyClass.base(this, 'constructor'); > > org.apache.royale.utils.Language.trace(this._foo_$P2); > > }; > > org.apache.royale.core.MyClass.prototype._foo_$P2 = "bar!"; > > > > (ommitted some output for clarity) > > > > That correctly separates both private foo members at the two > levels, > > and > > gives the expected output. > > > > But I need to understand what the concerns are that you > mentioned about > > swcs, Alex. You've using globally unique names for the solution > which > > is > > always going to work, I'm trying to find a way to limit the > solution > > to the > > nature of the conflicts which exist only in the inheritance > chain - > > 'depth' > > in the chain (the delta from 'Object' as a base class) seemed > like a > > good > > modifier for a 'private namespace', because in theory it would > never > > conflict with ancestors. > > I guess some stale swcs where swcs get updated independently and > where > > class hierarchies change in one that another depends on could > end up > > with > > conflicts reappearing. The above approach could be an issue if > one > > older > > swc uses a base class from another swc that became modified and > gained > > a > > longer inheritance chain than it did when the first swc was > made. Is > > that > > the type of thing you meant? > > > > > > > > > > > > On Tue, Nov 13, 2018 at 6:38 AM Alex Harui > <[email protected]> > > wrote: > > > > > Maybe. Certainly at the time we compile it, we don't know if > there > > will > > > be a subclass. > > > > > > I hopefully put in a check so that you can't "hide" a public or > > protected > > > API with a private one, but I think I left it so that a > subclass can > > create > > > a public API with the same name as a private API. I don't > know if > > that was > > > actually allowed in SWF, maybe you can test it out. But if > that is > > > allowed, then the "top-level" must have private APIs with > unique > > names. > > > > > > Of course, I could be wrong... > > > -Alex > > > > > > On 11/12/18, 7:27 AM, "Harbs" <[email protected]> wrote: > > > > > > I’m seeing private vars that are not subclassed also with > > qualified > > > names. It seems to me that top-level private vars have no > reason to > > be > > > qualified.My $0.02, > > > Harbs > > > > > > > On Nov 6, 2018, at 11:03 PM, Alex Harui > > <[email protected]> > > > wrote: > > > > > > > > It would probably work, but why chase the inheritance > chain at > > all? > > > > > > > > Go ahead and add that if you really want to, but I would > > rather we > > > not use colliding private names in the framework, and only one > > person spoke > > > up to say they use colliding private names in their code. > > > > > > > > Historically, folks really hated any private APIs in the > Flex > > > framework. Really, we should only use them as backing > variables of > > > getter/setters and for APIs we aren't sure we want to support > going > > > forward. And even the backing variables may get exposed as > > protected (some > > > already are). Over time, private API implementations should > > stabilize and > > > then should be supported by making them protected. > > > > > > > > So, do what you want to do, but I'm not sure how many > customers > > > there are. And if you do try to do this, make sure you can > walk the > > entire > > > hierarchy across SWCs. I think it should work, but that could > be a > > place > > > where you get stuck. > > > > > > > > Thanks, > > > > -Alex > > > > > > > > On 11/6/18, 11:30 AM, "Greg Dove" <[email protected]> > wrote: > > > > > > > > 'I'm not sure it is worth figuring out how far up the > > inheritance > > > chain the > > > > other private variable is and assigning numbers ' > > > > > > > > I was just meaning at simple class level - so not > doing > > anything > > > more than > > > > that. The 'depth number' is the same for all private > fields > > > > Y extends Object > > > > X extends Y > > > > > > > > in Y all privates are _$P1 suffix > > > > in X all privates are _$P2 suffix > > > > > > > > nothing too sophisticated > > > > > > > > would that work? > > > > > > > > > > > > On Wed, Nov 7, 2018 at 7:39 AM Alex Harui > > > <[email protected]> wrote: > > > > > > > >> Hi Greg, > > > >> > > > >> #1 is a good idea and will help, but the value for that > > variable is > > > still > > > >> pushed far to the right in the debugger and pushed > other code > > far > > > to the > > > >> right in the source window. > > > >> > > > >> #2 and similar ideas will help. I'm not sure it is > worth > > figuring > > > out how > > > >> far up the inheritance chain the other private variable > is and > > > assigning > > > >> numbers. I considered using some sort of hash of the > fully > > > qualified > > > >> classname so each prefix/suffix would be, say 8 > characters > > long. > > > >> > > > >> That said, though, in the framework, I think we should > not > > have > > > private > > > >> name collisions. I found some duplicate code that > would be > > caught > > > if we > > > >> turned on errors for these collisions, and would > encourage us > > to use > > > >> overriding which enables other folks to do overrides. > > > >> > > > >> So, if you want to muck around in this code, feel free > to > > switch > > > the order > > > >> in #1, but I think the next piece of work is to turn on > > errors for > > > >> collisions in the royale-asjs build. > > > >> > > > >> My 2 cents, > > > >> -Alex > > > >> > > > >> On 11/6/18, 10:07 AM, "Greg Dove" <[email protected]> > > wrote: > > > >> > > > >> Hi Alex, > > > >> > > > >> "IMO, it makes debugging the framework really > painful." > > > >> > > > >> I feel your pain. I didn't try switching it off yet, > but I > > > wonder if > > > >> it can > > > >> be on by default with a different approach: > > > >> example: > > > >> > > > >> > > > >> > > > > > > this.org_apache_royale_jewel_beads_models_DropDownListModel__selectedIndex > > > >> > > > >> 1. have the 'private' part of the name as a suffix > instead > > of > > > prefix. > > > >> It > > > >> means you can read the important part easier > (doesn't help > > with > > > the > > > >> very > > > >> long lines with multiple references on them, but > might be > > easier > > > in > > > >> general) > > > >> > > > >> 2. maybe the class name approach is not needed? > > > >> the problem domain is more about conflicts in > inheritance > > chain > > > rather > > > >> than > > > >> class naming, is it not? > > > >> so if > > > >> -local private name is __selectedIndex > > > >> -and the current class is 5 levels away from Object > base > > class > > > >> would it not be possible to use t > > > >> this.__selectedIndex_$P5 > > > >> or similar? > > > >> > > > >> if this 2nd part makes sense (still pondering it) > then it > > could > > > >> probably be > > > >> on by default. > > > >> > > > >> > > > >> > > > >> On Thu, Nov 1, 2018 at 5:21 AM Alex Harui > > > <[email protected]> > > > >> wrote: > > > >> > > > >>> Hi Kenny, > > > >>> > > > >>> I went and made the changes to decorate private > variable > > names. I > > > >> made it > > > >>> an option that's on by default. IMO, it makes > debugging the > > > >> framework > > > >>> really painful. I'm probably going to turn the flag > off in > > the > > > >> framework > > > >>> and have a policy that we don't use the same private > variable > > > >> names. It > > > >>> shouldn't affect what users do. > > > >>> > > > >>> Also, turning the option off revealed a fair amount of > > duplicated > > > >> code. > > > >>> Sometimes we copy a file to form the basis of a > subclass and > > then > > > >> don't > > > >>> clean up everything and you don't find out about > private > > methods if > > > >> they > > > >>> have their names decorated. > > > >>> > > > >>> -Alex > > > >>> > > > >>> On 10/28/18, 5:22 AM, "Kenny Lerma" < > [email protected]> > > wrote: > > > >>> > > > >>> Alex, this is absolutely needed. The same private > > variables > > > >> names in > > > >>> base > > > >>> class and subclass are quite common. Since this > only > > affects > > > >> debug, > > > >>> there > > > >>> is no harm in the decorator and maintains > consistency with > > > flash. > > > >>> > > > >>> > > > >>> On Sat, Oct 27, 2018, 9:05 PM Alex Harui > > > >> <[email protected]> > > > >>> wrote: > > > >>> > > > >>>> Hi, > > > >>>> > > > >>>> It appears that in Flash, private variables are > actually in > > a > > > >> custom > > > >>>> namespace. This means you can have private APIs in a > base > > > >> class and > > > >>>> private APIs in a subclass with the same name (and > aren't > > > >> overrides) > > > >>> and > > > >>>> everything "works". IOW: > > > >>>> > > > >>>> package org.apache.royale.core { > > > >>>> public class BaseClass { > > > >>>> private var foo:String = "foo,"; > > > >>>> public function BaseClass() { > > > >>>> trace(foo); > > > >>>> } > > > >>>> }}} > > > >>>> > > > >>>> package org.apache.royale.core { > > > >>>> public class MyClass { > > > >>>> private var foo:String = "bar!"; > > > >>>> public function MyClass() { > > > >>>> super(); > > > >>>> trace(foo); > > > >>>> } > > > >>>> }}} > > > >>>> > > > >>>> var baz:MyClass = new MyClass(); // outputs foo,bar! > > > >>>> > > > >>>> This is true for private functions and > getters/setters as > > well. > > > >>> However, > > > >>>> this currently does not work in JS, since the > transpiled > > code > > > >> looks > > > >>> like: > > > >>>> > > > >>>> org.apache.royale.core.BaseClass = function() { > > > >> trace(this.foo) } > > > >>>> org.apache.royale.core BaseClass.prototype.foo = > "foo,"; > > > >>>> > > > >>>> And > > > >>>> > > > >>>> org.apache.royale.core MyClass = function() { > > trace(this.foo} } > > > >>>> org.apache.royale.core MyClass.prototype.foo = "bar!"; > > > >>>> > > > >>>> So you will get "bar!bar!"; > > > >>>> > > > >>>> > > > >>>> The MX Charts code uses the same private API names in > base > > > >> classes > > > >>> and > > > >>>> subclasses. I don't know why they didn't use > protected > > > >> methods and > > > >>>> override them, so I'm going to change the Charts code > to use > > > >>> overrides just > > > >>>> to keep making progress. > > > >>>> > > > >>>> I'm wondering if anybody else uses the same private > API name > > > >> in base > > > >>>> classes and subclasses. The theoretical fix is to > have the > > > >> compiler > > > >>>> generate a decorated name. That's what the compiler > already > > > >> does for > > > >>>> mx_internal APIs and other custom namespace APIs, but > I > > think > > > >> it > > > >>> would make > > > >>>> our code fatter and uglier to decorate private API > names, so > > > >> I'm > > > >>> tempted to > > > >>>> have the compiler emit an error or warning instead. > > > >>>> > > > >>>> In order to guarantee uniqueness, we'd have to > decorate with > > > >> the > > > >>> fully > > > >>>> qualified name of the class. Then the transpiled > code would > > > >> look > > > >>> like: > > > >>>> > > > >>>> org.apache.royale.core.BaseClass = function() { > > > >>>> trace(this.org_apache_royale_core_BaseClass_foo) } > > > >>>> org.apache.royale.core > > > >>>> > BaseClass.prototype.org_apache_royale_core_BaseClass_foo = > > > >> "foo,"; > > > >>>> > > > >>>> And > > > >>>> > > > >>>> org.apache.royale.core MyClass = function() { > > > >>>> trace(this.org_apache_royale_core_MyClass_foo} } > > > >>>> org.apache.royale.core > > > >>>> MyClass.prototype.org_apache_royale_core_MyClass_foo = > > "bar!"; > > > >>>> > > > >>>> IMO, that's a painful change to the transpiler, so I > want to > > > >> make > > > >>> sure we > > > >>>> really need to do this. I think it won't impact > minified > > > >> size, but > > > >>> it will > > > >>>> be noticeable when debugging the JS. > > > >>>> > > > >>>> Thoughts? > > > >>>> -Alex > > > >>>> > > > >>>> > > > >>>> > > > >>> > > > >>> > > > >>> > > > >> > > > >> > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
