Skip to content

Commit

Permalink
Issue #1356 ... range comprehension optimization when a step is present.
Browse files Browse the repository at this point in the history
  • Loading branch information
jashkenas committed May 15, 2011
1 parent 9e32a5b commit c056c93
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 74 deletions.
67 changes: 25 additions & 42 deletions lib/nodes.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

71 changes: 39 additions & 32 deletions src/nodes.coffee
Expand Up @@ -632,41 +632,47 @@ exports.Range = class Range extends Base
# Compiles the range's source variables -- where it starts and where it ends.
# But only if they need to be cached to avoid double evaluation.
compileVariables: (o) ->
o = merge(o, top: true)
[@from, @fromVar] = @from.cache o, LEVEL_LIST
[@to, @toVar] = @to.cache o, LEVEL_LIST
[@fromNum, @toNum] = [@fromVar.match(SIMPLENUM), @toVar.match(SIMPLENUM)]
parts = []
parts.push @from if @from isnt @fromVar
parts.push @to if @to isnt @toVar
o = merge o, top: true
[@from, @fromVar] = @from.cache o, LEVEL_LIST
[@to, @toVar] = @to.cache o, LEVEL_LIST
[@step, @stepVar] = step.cache o, LEVEL_LIST if step = del o, 'step'
[@fromNum, @toNum] = [@fromVar.match(SIMPLENUM), @toVar.match(SIMPLENUM)]
@stepNum = @stepVar.match(SIMPLENUM) if @stepVar

# When compiled normally, the range returns the contents of the *for loop*
# needed to iterate over the values in the range. Used by comprehensions.
compileNode: (o) ->
@compileVariables o
@compileVariables o unless @fromVar
return @compileArray(o) unless o.index
return @compileSimple(o) if @fromNum and @toNum

# Set up endpoints.
known = @fromNum and @toNum
idx = del o, 'index'
step = del o, 'step'
stepvar = o.scope.freeVariable "step" if step
varPart = "#{idx} = #{@from}" + ( if @to isnt @toVar then ", #{@to}" else '' ) + if step then ", #{stepvar} = #{step.compile(o)}" else ''
cond = "#{@fromVar} <= #{@toVar}"
condPart = "#{cond} ? #{idx} <#{@equals} #{@toVar} : #{idx} >#{@equals} #{@toVar}"
stepPart = if step then "#{idx} += #{stepvar}" else "#{cond} ? #{idx}++ : #{idx}--"
"#{varPart}; #{condPart}; #{stepPart}"

# Compile a simple range comprehension, with integers.
compileSimple: (o) ->
[from, to] = [+@fromNum, +@toNum]
idx = del o, 'index'
step = del o, 'step'
stepvar = o.scope.freeVariable "step" if step
varPart = "#{idx} = #{from}"
varPart += ", #{stepvar} = #{step.compile(o)}" if step
condPart = if from <= to then "#{idx} <#{@equals} #{to}" else "#{idx} >#{@equals} #{to}"
stepPart = "#{idx} += #{stepvar}" if step
stepPart = ( if from <= to then "#{idx}++" else "#{idx}--" ) if not step
varPart = "#{idx} = #{@from}"
varPart += ", #{@to}" if @to isnt @toVar
varPart += ", #{@step}" if @step isnt @stepVar

# Generate the condition.
condPart = if @stepNum
condPart = if +@stepNum > 0 then "#{idx} <#{@equals} #{@toVar}" else "#{idx} >#{@equals} #{@toVar}"
else if known
[from, to] = [+@fromNum, +@toNum]
condPart = if from <= to then "#{idx} <#{@equals} #{to}" else "#{idx} >#{@equals} #{to}"
else
cond = "#{@fromVar} <= #{@toVar}"
condPart = "#{cond} ? #{idx} <#{@equals} #{@toVar} : #{idx} >#{@equals} #{@toVar}"

# Generate the step.
stepPart = if @stepVar
"#{idx} += #{@stepVar}"
else if known
if from <= to then "#{idx}++" else "#{idx}--"
else
"#{cond} ? #{idx}++ : #{idx}--"

# The final loop body.
"#{varPart}; #{condPart}; #{stepPart}"


# When used as a value, expand the range into the equivalent array.
compileArray: (o) ->
Expand All @@ -680,7 +686,7 @@ exports.Range = class Range extends Base
pre = "\n#{idt}#{result} = [];"
if @fromNum and @toNum
o.index = i
body = @compileSimple o
body = @compileNode o
else
vars = "#{i} = #{@from}" + if @to isnt @toVar then ", #{@to}" else ''
cond = "#{@fromVar} <= #{@toVar}"
Expand Down Expand Up @@ -1222,9 +1228,7 @@ exports.While = class While extends Base
# Simple Arithmetic and logical operations. Performs some conversion from
# CoffeeScript operations into their JavaScript equivalents.
exports.Op = class Op extends Base




constructor: (op, first, second, flip, @isExistentialEquals ) ->
return new In first, second if op is 'in'
if op is 'do'
Expand Down Expand Up @@ -1257,6 +1261,9 @@ exports.Op = class Op extends Base

isUnary: ->
not @second

isComplex: ->
not (@isUnary() and (@operator in ['+', '-'])) or @first.isComplex()

This comment has been minimized.

Copy link
@satyr

satyr May 15, 2011

Collaborator

Why opting out +/- again? They aren't side-effect free:

$ bin/coffee -e 'n = valueOf: (-> console.log "!"; 1); 0 < +n < 2'
!
!

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra May 25, 2011

Collaborator

Agreed. @jashkenas: are you going to address this?

This comment has been minimized.

Copy link
@jashkenas

jashkenas May 25, 2011

Author Owner

I wasn't planning on it -- abuse of valueOf isn't something we need to enable. @michaelficarra: would you disagree?

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra May 25, 2011

Collaborator

@jashkenas: I almost always do consider valueOf side-effects. I like contributing to mishoo/UglifyJS, and we are constantly making provisions for valueOf side-effects. If we did not, we could make many more substitutions. Since CS takes the stance that "it's just JS", I think we need to account for the possibility of code like @satyr's. Just last week, I suggested on the IRC channel an abuse of the valueOf property for the piping DSL (something like a | b c | d e f). Though my suggestion did not include side-effects, so it's up to you. That case may be much less common.

I'd like to hear feedback from more CS users. Anyone have an opinion on this?

This comment has been minimized.

Copy link
@jashkenas

jashkenas May 25, 2011

Author Owner

Yes -- more feedback would be appreciated. My personal experience is that in all the places valueOf() is used in the wild -- which are few and far between -- all of the acceptable use cases are entirely side-effect-free. It's kind of built-in to the name of the function: if your valueOf() causes a side effect, that's a bug.

This comment has been minimized.

Copy link
@satyr

satyr May 25, 2011

Collaborator

Don't forget toString, which is more commonly used and can be costly even when untouched (e.g. Function::toString with large functions).

I'd like to hear feedback from more CS users. Anyone have an opinion on this?

I doubt normal users read commit comments.

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra May 25, 2011

Collaborator

I doubt normal users read commit comments.

I believe all commit comments show up in the "news feed" of anyone that follows coffee-script, just like commits or newly-created issues. That would be over 2000 potential eyes. Can any non-collaborator confirm this?

This comment has been minimized.

Copy link
@geraldalewis

geraldalewis May 25, 2011

Contributor

Confirming: I follow coffee-script, and can/do read commit comments.

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra May 27, 2011

Collaborator

Alright, looks like nobody's responding. @jashkenas: @satyr: should I open an issue? I'm on the fence.

This comment has been minimized.

Copy link
@satyr

satyr May 27, 2011

Collaborator

Personally I no longer mind. Inconsistency can be a part of CoffeeScript philosophy I guess.

This comment has been minimized.

Copy link
@jashkenas

jashkenas May 27, 2011

Author Owner

Personally, I'm not worried about it. But if someone made a good use-case for a side-effect-ful valueOf, that actually appeared in real-world code, I'd change my mind.


# Am I capable of
# [Python-style comparison chaining](http://docs.python.org/reference/expressions.html#notin)?
Expand Down

0 comments on commit c056c93

Please sign in to comment.