Add @accessor support to class_addIvar. #1712

Closed
wants to merge 3 commits into
from

Projects

None yet

6 participants

@tolmasky
Member
tolmasky commented Jan 4, 2013

In preparation for the new parser/compiler, I have beefed up class_addIvar to support accessor generation. This is in contrast to how we currently handle accessor generation by actually inserting setter/getter code into the resultant javascript output.

Instead, class_addIvar will now support a new fourth parameter describing the accessors that should be supported. The fourth parameter is a JS object with the following (optional) properties:

{
"getter": string_name_of_getter_method,
"setter": string_name_of_setter_method,
"copy" : true_or_false_or_absent
}

For example:

class_addIvar(CPView, "_subviews", "CPArray", { "getter": "subviews", "setter": "setSubviews:", "copy":true });

class_addIvar will then internally generate the methods and add them using class_addMethod. There are a few reasons for this:

  1. If we ever decide to change the behavior of accessor generation, this will avoid different frameworks having "stale" versions of getter/setters. For example, right now we only pay attention to "copy" with setters. I think its worth considering also returning a copy for getters. With our current implementation, this change would require recompiling the world.
  2. Smaller resultant code. Accessor code is completely redundant, yet has the potential to be a significant portion of what we push over the wire if we start using it in ernest. There is no reason for this when we can describe it much more concisely.
  3. This allows for easier dynamic setter/getter generation if you are making a class at runtime.
  4. This is closer to how properties work in ObjC (class_addProperty)

However, the main impetus behind this was to allow for line code parity with the new parser/compiler. What this means is that every line of generated code is guaranteed to be on the same line as the original code. This means if you get a runtime error on line 56, you can be sure to look at the same code in your original code files. This is helpful in environments where we won't have source maps. Everything else was being generated on the same line, except for the fact that all the ivars were being bunched up together with class_addIvar_s_ and all the generated methods. This allows us to inline the class_addIvar, and not require any additional code (thus offsetting anything after it) for accessors. After the transition is complete, we should deprecate class_addIvar_s_ (and class_addMethod_s_).

@cappbot
cappbot commented Jan 4, 2013

Milestone: Someday. Label: #new. What's next? A reviewer should examine this issue.

@aljungberg aljungberg and 1 other commented on an outdated diff Jan 4, 2013
Objective-J/Runtime.js
@@ -155,11 +155,27 @@ GLOBAL(class_addIvar) = function(/*Class*/ aClass, /*String*/ aName, /*String*/
thePrototype[aName] = NULL;
+ if (accessors)
+ {
+ var copy = accessors.copy || false;
+ var access = "self." + ivar;
+
+ if (hasOwnProperty.call(accessors, "setter"))
+ class_addMethod(aClass, accessors["setter"], new Function("self", "_cmd", "anObject", copy ?
+ access " = anObject;" :
@aljungberg
aljungberg Jan 4, 2013 Member

Shouldn't this be access + " = anObject;"?

And shouldn't it be !copy ? ... ?

@tolmasky
tolmasky Jan 4, 2013 Member

Sorry just noticed I had more changes sitting in diff that hadn't been committed, trying to figure out how to add them to the pull request now.

On Jan 4, 2013, at 11:13 AM, Alexander Ljungberg notifications@github.com wrote:

In Objective-J/Runtime.js:

@@ -155,11 +155,27 @@ GLOBAL(class_addIvar) = function(/Class/ aClass, /String/ aName, /String/

 thePrototype[aName] = NULL;
  • if (accessors)
  • {
  •    var copy = accessors.copy || false;
    
  •    var access = "self." + ivar;
    
  •    if (hasOwnProperty.call(accessors, "setter"))
    
  •        class_addMethod(aClass, accessors["setter"], new Function("self", "_cmd", "anObject", copy ?
    
  •            access " = anObject;" :
    
    Shouldn't this be access + " = anObject;"?

And shouldn't it be !copy ? ... ?


Reply to this email directly or view it on GitHub.

@tolmasky
tolmasky Jan 4, 2013 Member

OK looks like the commit got added automatically.

On Jan 4, 2013, at 11:13 AM, Alexander Ljungberg notifications@github.com wrote:

In Objective-J/Runtime.js:

@@ -155,11 +155,27 @@ GLOBAL(class_addIvar) = function(/Class/ aClass, /String/ aName, /String/

 thePrototype[aName] = NULL;
  • if (accessors)
  • {
  •    var copy = accessors.copy || false;
    
  •    var access = "self." + ivar;
    
  •    if (hasOwnProperty.call(accessors, "setter"))
    
  •        class_addMethod(aClass, accessors["setter"], new Function("self", "_cmd", "anObject", copy ?
    
  •            access " = anObject;" :
    
    Shouldn't this be access + " = anObject;"?

And shouldn't it be !copy ? ... ?


Reply to this email directly or view it on GitHub.

@tolmasky
Member
tolmasky commented Jan 4, 2013

OK I guess it added it automatically.

@aljungberg
Member

So copy on get could be expensive so maybe we should leave that out and just rely on people not to modify internal arrays/objects.

+milestone=0.9.6

+feature
+Objective-J

@cappbot
cappbot commented Jan 4, 2013

Milestone: Someday. Labels: #new, Objective-J, feature. What's next? A reviewer should examine this issue.

@tolmasky
Member
tolmasky commented Jan 4, 2013

So back in the day, we had to have a huge purge to fix all the bugs resulting from getting an array, mutating it, then everything being broken from then on (the most notable candidate was -subviews). It seems that it is just asking for a bug when doing that. We certainly wouldn't be able to change a lot of existing accesses to accessors with this current method. It would at least be nice to have a way to specify it copyget or something.

@aljungberg
Member

Okay I agree with that, copyget would be nice.

@tolmasky
Member
tolmasky commented Jan 4, 2013

maybe we could have copy=set/get/both, and if you just do "copy" it is the same as "get" for legacy

@Me1000
Contributor
Me1000 commented Jan 4, 2013

Why would you ever want to getcopy but not setcopy?

@tolmasky
Member
tolmasky commented Jan 4, 2013

well, it would be in the case where there is not setter. like @accessors(readonly, copy=get)

@Me1000
Contributor
Me1000 commented Jan 4, 2013

That just seems redundant.
@accessor means generate getters/setters.
readonly says don't generate setters
copy means you have a clean copy of the object either being assigned or returned.

RE: Objective-C @property's copy behavior. It is confusing and terrible. We should deviate from that.

@tolmasky
Member
tolmasky commented Jan 4, 2013

OK, that was the original suggestions, but aljungberg said you cold get lots of copies you didn't intend possibly. So we have the following issues:

  1. Currently we do NOT copy on get, changing that behavior may have unintended consequences (not that that's ever stopped us before).
  2. Do we want to a way to have copyset but NOT copyget.

If either 1 or 2 is a concern, then we need a way to specify all possible options: no-copy, copyget, copyset, copyboth.

@aljungberg
Member

So the concern is that copy on get could be very expensive. Imagine if you're getting a huge cache dictionary or something. So maybe you want copyset but not copyget to avoid that hit while still getting the basic level of safety from copyset

@tolmasky
Member
tolmasky commented Jan 4, 2013

Alternatively we could try to be smart like Cocoa theoretically is, and have "smart copies". So you say [mutable_dictionary copy] and it returns a wrapper thats not an actual copy until the underlying dictionary actually changes.

@boucher
Member
boucher commented Jan 4, 2013

we could but only for non toll free bridged objects

On Fri, Jan 4, 2013 at 1:26 PM, Francisco Ryan Tolmasky I <
notifications@github.com> wrote:

Alternatively we could try to be smart like Cocoa theoretically is, and
have "smart copies". So you say [mutable_dictionary copy] and it returns a
wrapper thats not an actual copy until the underlying dictionary actually
changes.


Reply to this email directly or view it on GitHubhttps://github.com/cappuccino/cappuccino/pull/1712#issuecomment-11900505.

@Me1000
Contributor
Me1000 commented Jan 4, 2013

I don't think additional copies are really going to be as big of an issue as it sounds like.
Although hardly scientific I did a quick benchmark:
Using the same copy method CPArray uses, I copied a native JS array of 100,000 strings 100 times…
it took 0.06s. Also, this is not a linear relationship. Making more copies of an array with fewer objects was WAY faster.
That's not to say we shouldn't be concerned about it, but rather that I'm sure there are plenty of areas we could get better performance improvements if we looked hard enough. Also, I'm pretty sure we already have a number of unnecessary copies, so I don't think this will be a major leap by any means.

I agree that changing the behavior sucks, since it's already been around so long. But, I also imagine there are a number of undiscovered issues resulting from the current (confusing) copy behavior.

If we're worried about compatibility, I suggest throwing some warnings when "copy" is used, deprecating it, and going with setcopy, getcopy, bothcopy.

or consider this a change with objective-j 2.0, Clearly document it, and have some kind of static tool that identifies potential problems in existing code based off of changes in the language. Something that might be valuable anyway, since we're already changing the lookup order slightly for identifiers by removing the with() statement.

@tolmasky tolmasky closed this Jan 6, 2013
@aljungberg
Member

I see this is closed now, did we decide against this?

@tolmasky
Member

I simply accidentally deleted the branch (still have the code though). Ill pull request it again soon

Sent from my iPhone

On Jan 17, 2013, at 9:19 AM, Alexander Ljungberg notifications@github.com wrote:

I see this is closed now, did we decide against this?


Reply to this email directly or view it on GitHub.

@aljungberg aljungberg referenced this pull request Feb 14, 2013
Closed

@accessors lose type #1008

@Ioprst
Ioprst commented Sep 23, 2013

wefdd

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