metalet with postfix improvement

metalet with postfix improvement

[vsm, 2015-04-09 16:58:27]

vsm@google.com changed reviewers:
+ vsm@google.com

[vsm, 2015-04-09 16:58:28]

https://codereview.chromium.org/1071773002/diff/20001/lib/runtime/dart/_isola...
File lib/runtime/dart/_isolate_helper.js (right):

https://codereview.chromium.org/1071773002/diff/20001/lib/runtime/dart/_isola...
lib/runtime/dart/_isolate_helper.js:361:
dart.prop(exports._globalState.topEventLoop, _activeJsAsyncCount).notNull++;
How about just specializing the increment case?  E.g.,

dart.dincrement(exports._globalState.topEventLoop, _activeJsAsyncCount)

with optional args for increment (1 by default) and perhaps pre/post (post by
default):

dart.dincrement(exports._globalState.topEventLoop, _activeJsAsyncCount, -1)

?

[Jennifer Messerly, 2015-04-09 17:47:40]

https://codereview.chromium.org/1071773002/diff/20001/lib/runtime/dart/_isola...
File lib/runtime/dart/_isolate_helper.js (right):

https://codereview.chromium.org/1071773002/diff/20001/lib/runtime/dart/_isola...
lib/runtime/dart/_isolate_helper.js:361:
dart.prop(exports._globalState.topEventLoop, _activeJsAsyncCount).notNull++;
On 2015/04/09 16:58:28, vsm wrote:
> How about just specializing the increment case?  E.g.,
> 
> dart.dincrement(exports._globalState.topEventLoop, _activeJsAsyncCount)
> 
> with optional args for increment (1 by default) and perhaps pre/post (post by
> default):
> 
> dart.dincrement(exports._globalState.topEventLoop, _activeJsAsyncCount, -1)
> 
> ?

yeah, that could work, would we have different prefix vs postfix variants, or
only postifx?

Note the diff in this CL is a bit misleading, I only added support for postfix
so far, so prefix/op assign won't show up.

[vsm, 2015-04-10 00:14:11]

https://codereview.chromium.org/1071773002/diff/20001/lib/runtime/dart/_isola...
File lib/runtime/dart/_isolate_helper.js (right):

https://codereview.chromium.org/1071773002/diff/20001/lib/runtime/dart/_isola...
lib/runtime/dart/_isolate_helper.js:361:
dart.prop(exports._globalState.topEventLoop, _activeJsAsyncCount).notNull++;
On 2015/04/09 17:47:40, John Messerly wrote:
> On 2015/04/09 16:58:28, vsm wrote:
> > How about just specializing the increment case?  E.g.,
> > 
> > dart.dincrement(exports._globalState.topEventLoop, _activeJsAsyncCount)
> > 
> > with optional args for increment (1 by default) and perhaps pre/post (post
by
> > default):
> > 
> > dart.dincrement(exports._globalState.topEventLoop, _activeJsAsyncCount, -1)
> > 
> > ?
> 
> yeah, that could work, would we have different prefix vs postfix variants, or
> only postifx?
> 
> Note the diff in this CL is a bit misleading, I only added support for postfix
> so far, so prefix/op assign won't show up.

Another alternative:

dart.nullcheck(exports._globalState.topEventLoop, _activeJsAsyncCount)++;

i.e., fold the notNull and prop into one since it's such a common pattern.

In general, the fact that x.notNull++ changes x is surprising to me.

[Jennifer Messerly, 2015-04-10 01:00:24]

https://codereview.chromium.org/1071773002/diff/20001/lib/runtime/dart/_isola...
File lib/runtime/dart/_isolate_helper.js (right):

https://codereview.chromium.org/1071773002/diff/20001/lib/runtime/dart/_isola...
lib/runtime/dart/_isolate_helper.js:361:
dart.prop(exports._globalState.topEventLoop, _activeJsAsyncCount).notNull++;
On 2015/04/10 00:14:10, vsm wrote:
> On 2015/04/09 17:47:40, John Messerly wrote:
> > On 2015/04/09 16:58:28, vsm wrote:
> > > How about just specializing the increment case?  E.g.,
> > > 
> > > dart.dincrement(exports._globalState.topEventLoop, _activeJsAsyncCount)
> > > 
> > > with optional args for increment (1 by default) and perhaps pre/post (post
> by
> > > default):
> > > 
> > > dart.dincrement(exports._globalState.topEventLoop, _activeJsAsyncCount,
-1)
> > > 
> > > ?
> > 
> > yeah, that could work, would we have different prefix vs postfix variants,
or
> > only postifx?
> > 
> > Note the diff in this CL is a bit misleading, I only added support for
postfix
> > so far, so prefix/op assign won't show up.
> 
> Another alternative:
> 
> dart.nullcheck(exports._globalState.topEventLoop, _activeJsAsyncCount)++;
> 
> i.e., fold the notNull and prop into one since it's such a common pattern.


that won't work though ... it has to be an lvalue. The ".notNull" is so we get
an lvalue.

it could be: `dart.nullcheck(obj, prop).value` ... that was just more wordy,
only reason I didn't go for it.

> 
> In general, the fact that x.notNull++ changes x is surprising to me.
> 

yeah, it's all degrees of terrible, IMHO =\

[Jennifer Messerly, 2015-04-10 16:56:47]

let's close this one for now, and revisit once we have a better idea if the
notNull calls will be a problem :)