Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(833)

Issue 74543003: Split out Index AST node from Invoke (Closed)

Created:
7 years, 1 month ago by justinfagnani
Modified:
7 years, 1 month ago
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Split out Index AST node from Invoke BUG= R=jmesserly@google.com Committed: https://code.google.com/p/dart/source/detail?r=30437

Patch Set 1 #

Patch Set 2 : Split out Getter too #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+227 lines, -114 lines) Patch
M pkg/polymer_expressions/benchmark/eval.dart View 1 1 chunk +29 lines, -17 lines 2 comments Download
M pkg/polymer_expressions/lib/eval.dart View 1 4 chunks +114 lines, -65 lines 3 comments Download
M pkg/polymer_expressions/lib/expression.dart View 1 5 chunks +53 lines, -8 lines 0 comments Download
M pkg/polymer_expressions/lib/parser.dart View 1 5 chunks +8 lines, -13 lines 0 comments Download
M pkg/polymer_expressions/lib/visitor.dart View 1 2 chunks +13 lines, -0 lines 0 comments Download
M pkg/polymer_expressions/test/parser_test.dart View 1 4 chunks +10 lines, -11 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
justinfagnani
Doesn't move the perf needle, but a good cleanup. I'll split property/getter access next
7 years, 1 month ago (2013-11-16 02:39:57 UTC) #1
Jennifer Messerly
lgtm https://codereview.chromium.org/74543003/diff/40001/pkg/polymer_expressions/benchmark/eval.dart File pkg/polymer_expressions/benchmark/eval.dart (right): https://codereview.chromium.org/74543003/diff/40001/pkg/polymer_expressions/benchmark/eval.dart#newcode34 pkg/polymer_expressions/benchmark/eval.dart:34: var score = new EvalBenchmark(name, expr, model: model, ...
7 years, 1 month ago (2013-11-18 20:28:41 UTC) #2
justinfagnani
On 2013/11/18 20:28:41, John Messerly wrote: > lgtm > > https://codereview.chromium.org/74543003/diff/40001/pkg/polymer_expressions/benchmark/eval.dart > File pkg/polymer_expressions/benchmark/eval.dart (right): ...
7 years, 1 month ago (2013-11-20 02:23:16 UTC) #3
justinfagnani
Committed patchset #2 manually as r30437 (presubmit successful).
7 years, 1 month ago (2013-11-20 03:47:10 UTC) #4
justinfagnani
https://chromiumcodereview.appspot.com/74543003/diff/40001/pkg/polymer_expressions/benchmark/eval.dart File pkg/polymer_expressions/benchmark/eval.dart (right): https://chromiumcodereview.appspot.com/74543003/diff/40001/pkg/polymer_expressions/benchmark/eval.dart#newcode34 pkg/polymer_expressions/benchmark/eval.dart:34: var score = new EvalBenchmark(name, expr, model: model, variables: ...
7 years, 1 month ago (2013-11-20 03:50:33 UTC) #5
Jennifer Messerly
7 years, 1 month ago (2013-11-20 04:04:55 UTC) #6
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/74543003/diff/40001/pkg/polymer_expres...
File pkg/polymer_expressions/lib/eval.dart (right):

https://chromiumcodereview.appspot.com/74543003/diff/40001/pkg/polymer_expres...
pkg/polymer_expressions/lib/eval.dart:85: * literal index.
On 2013/11/20 03:50:33, justinfagnani wrote:
> On 2013/11/18 20:28:41, John Messerly wrote:
> > out of curiosity, why have the restriction that the index must be literal?
is
> it
> > just not implemented?
> 
> No, I was just being conservative at the time and wanted to make sure that we
> knew which object we were assigning too. Some cases, like f(a) and (a + 2) are
> clearly not assignable, while f(a).name might be. This simplest restriction I
> could come up with property paths, but supporting a.b[3].c seemed very useful
> not far from property paths. The more general restriction is that we need to
be
> able to separate the expression into the owner and the property and right not
> the property must be a name or constant index value. We can relax this so that
> the property can be an expression that's evaluated and then as long as we can
> separate the expression, it's assignable. f(a) still wouldn't work, but
> f(a).name would, and a[b].

makes sense. yeah, I think we'd eventually want all lvalues to be supported,
where lvalue is either:

name  (variable)
expr.name (property access)
expr[expr] (indexing)

Powered by Google App Engine
This is Rietveld 408576698