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.
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. 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 > > >>>> > > >>>> > > >>>> > > >>> > > >>> > > >>> > > >> > > >> > > >> > > > > > > > > > > > > > > > > >
