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

Issue 10915022: Implement argument definition test in the vm. (Closed)

Created:
8 years, 3 months ago by regis
Modified:
8 years, 3 months ago
Reviewers:
srdjan, hausner
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Implement argument definition test in the vm. Add tests. Various cleanups. Committed: https://code.google.com/p/dart/source/detail?r=11664

Patch Set 1 #

Total comments: 4

Patch Set 2 : #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+505 lines, -28 lines) Patch
M runtime/lib/object.cc View 1 1 chunk +1 line, -2 lines 0 comments Download
M runtime/vm/ast.h View 1 2 chunks +36 lines, -0 lines 0 comments Download
M runtime/vm/ast_printer.cc View 1 1 chunk +9 lines, -0 lines 0 comments Download
M runtime/vm/code_generator.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M runtime/vm/code_generator.cc View 1 1 chunk +32 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_builder.cc View 1 3 chunks +17 lines, -1 line 0 comments Download
M runtime/vm/flow_graph_compiler_ia32.cc View 1 2 chunks +17 lines, -1 line 0 comments Download
M runtime/vm/flow_graph_compiler_x64.cc View 1 2 chunks +17 lines, -1 line 0 comments Download
M runtime/vm/il_printer.cc View 1 1 chunk +8 lines, -0 lines 0 comments Download
M runtime/vm/intermediate_language.h View 1 2 chunks +33 lines, -0 lines 0 comments Download
M runtime/vm/intermediate_language.cc View 1 1 chunk +5 lines, -0 lines 0 comments Download
M runtime/vm/intermediate_language_ia32.cc View 1 1 chunk +29 lines, -0 lines 0 comments Download
M runtime/vm/intermediate_language_x64.cc View 1 1 chunk +29 lines, -0 lines 0 comments Download
M runtime/vm/parser.h View 1 3 chunks +9 lines, -0 lines 0 comments Download
M runtime/vm/parser.cc View 1 7 chunks +184 lines, -9 lines 2 comments Download
M runtime/vm/scopes.h View 1 4 chunks +2 lines, -6 lines 0 comments Download
M runtime/vm/scopes.cc View 1 4 chunks +4 lines, -7 lines 3 comments Download
M runtime/vm/symbols.h View 1 3 chunks +6 lines, -1 line 0 comments Download
M runtime/vm/symbols.cc View 1 1 chunk +6 lines, -0 lines 2 comments Download
A tests/language/argument_definition_test.dart View 1 chunk +57 lines, -0 lines 3 comments Download
M tests/language/language.status View 1 2 chunks +2 lines, -0 lines 0 comments Download
M tests/language/language_dart2js.status View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
regis
8 years, 3 months ago (2012-08-30 21:37:04 UTC) #1
srdjan
LGTM https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/ast.h File runtime/vm/ast.h (right): https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/ast.h#newcode250 runtime/vm/ast.h:250: explicit ArgumentDefinitionTestNode(intptr_t token_pos, remove explicit https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/flow_graph_compiler_ia32.cc File runtime/vm/flow_graph_compiler_ia32.cc ...
8 years, 3 months ago (2012-08-30 22:15:59 UTC) #2
regis
Thanks! https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/ast.h File runtime/vm/ast.h (right): https://chromiumcodereview.appspot.com/10915022/diff/1/runtime/vm/ast.h#newcode250 runtime/vm/ast.h:250: explicit ArgumentDefinitionTestNode(intptr_t token_pos, On 2012/08/30 22:15:59, srdjan wrote: ...
8 years, 3 months ago (2012-08-30 22:45:31 UTC) #3
hausner
LGTM. Parser/scope code looks very delicate... https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/parser.cc#newcode7437 runtime/vm/parser.cc:7437: UNREACHABLE(); Is this ...
8 years, 3 months ago (2012-08-31 00:34:55 UTC) #4
regis
Thanks! https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/parser.cc#newcode7437 runtime/vm/parser.cc:7437: UNREACHABLE(); On 2012/08/31 00:34:55, hausner wrote: > Is ...
8 years, 3 months ago (2012-08-31 01:36:18 UTC) #5
hausner
8 years, 3 months ago (2012-08-31 05:56:26 UTC) #6
https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/runtime/vm/scopes.c...
runtime/vm/scopes.cc:262: Symbols::Name(Symbols::kSavedEntryContextVar))) {
Ok, that's a valid reason that I buy. I hope Equals does not allocate a handle
(I don't have the source code handy).

On 2012/08/31 01:36:18, regis wrote:
> On 2012/08/31 00:34:55, hausner wrote:
> > Isn't it enough to just allocate a string handle for the symbol, which gets
> > allocated as a RawString.
> > 
> > ...Equals(String::Handle(Symbols::SavedEntryContextVar()))
> 
> Yes, but it seems like a waste to allocate all these handles in this recursive
> function just to access a compile time char*. This is probably why you had the
> static char* in the first place. I need also need a char* elsewhere for the
> arg_desc variable name and I thought that making all these predefined symbols
as
> char* was convenient. But if it causes any issue, let's revisit.

https://chromiumcodereview.appspot.com/10915022/diff/2004/tests/language/argu...
File tests/language/argument_definition_test.dart (right):

https://chromiumcodereview.appspot.com/10915022/diff/2004/tests/language/argu...
tests/language/argument_definition_test.dart:43: 
We had some good laughs in the office today. I haven't run this specific code,
but we did run similar ones and they worked. Feel free to use a different
function name :)

On 2012/08/31 01:36:18, regis wrote:
> On 2012/08/31 00:34:55, hausner wrote:
> > Can add this nice test function:
> > 
> > WTF([a, b, c]) {
> >   Expect.true(!?a??b:!?c == ?a?!?c:?b)
> > }
> > 
> 
> LOL
> Does it work? Does it matter how you call WTF()?

Powered by Google App Engine
This is Rietveld 408576698