Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1380383002:
Add support for configuration-specific imports to the VM. (Closed)
|
Created:
5 years, 2 months ago by floitsch Modified:
4 years, 11 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org, Ivan Posva Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd support for configuration-specific imports to the VM.
R=hausner@google.com, iposva@google.com
Committed: https://github.com/dart-lang/sdk/commit/e0fe0c0ed87995150583579e4329f00f3e89f38e
Patch Set 1 #Patch Set 2 : Reupload after merge #Patch Set 3 : Add flag. #
Total comments: 6
Patch Set 4 : Address comments. #
Total comments: 10
Patch Set 5 : Address comments and merge. #Patch Set 6 : Fix bad merge and some other fixes. #Patch Set 7 : Update status file. #
Total comments: 2
Patch Set 8 : Don't allocate symbols. #
Total comments: 5
Patch Set 9 : Address comments. #
Messages
Total messages: 21 (4 generated)
floitsch@google.com changed reviewers: + hausner@google.com
LGTM with comments. https://codereview.chromium.org/1380383002/diff/40001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/1380383002/diff/40001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5856: valueNode = ParseStringLiteral(false); Can't just assume that the next token is indeed a string literal. Please add proper error handling, e.g: CheckToken(Token::kSTRING, "string literal expected") https://codereview.chromium.org/1380383002/diff/40001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5859: AstNode* conditional_url_literal = ParseStringLiteral(false); Ditto. https://codereview.chromium.org/1380383002/diff/40001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5865: const String& value = valueNode == NULL Instead of computing the value here, you could declare it with a default value of "true" in line 5853 and then overwrite the default value if there is an explicit value in source code.
Patchset #4 (id:60001) has been deleted
Thanks. https://codereview.chromium.org/1380383002/diff/40001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/1380383002/diff/40001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5856: valueNode = ParseStringLiteral(false); On 2015/10/13 23:48:36, hausner wrote: > Can't just assume that the next token is indeed a string literal. Please add > proper error handling, e.g: > CheckToken(Token::kSTRING, "string literal expected") good catch. thanks. I mistakenly assumed the "ParseStringLiteral" would do it for me. done. https://codereview.chromium.org/1380383002/diff/40001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5859: AstNode* conditional_url_literal = ParseStringLiteral(false); On 2015/10/13 23:48:36, hausner wrote: > Ditto. Done. https://codereview.chromium.org/1380383002/diff/40001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5865: const String& value = valueNode == NULL On 2015/10/13 23:48:36, hausner wrote: > Instead of computing the value here, you could declare it with a default value > of "true" in line 5853 and then overwrite the default value if there is an > explicit value in source code. Tried to do it, but then I had to allocate a new handle, and explicitly get the 'raw' pointer from the True-symbol... In the end I kept it the way it is now.
iposva@google.com changed reviewers: + iposva@google.com
Do NOT commit. I need to see the spec first before we can move forward on this. Thanks, -Ivan
On 2015/10/20 07:24:22, Ivan Posva wrote: > Do NOT commit. I need to see the spec first before we can move forward on this. > > Thanks, > -Ivan ping.
On 2015/10/26 18:12:42, floitsch wrote: > On 2015/10/20 07:24:22, Ivan Posva wrote: > > Do NOT commit. I need to see the spec first before we can move forward on > this. > > > > Thanks, > > -Ivan > > ping. ping.
On 2015/10/29 10:24:40, floitsch wrote: > On 2015/10/26 18:12:42, floitsch wrote: > > On 2015/10/20 07:24:22, Ivan Posva wrote: > > > Do NOT commit. I need to see the spec first before we can move forward on > > this. > > > > > > Thanks, > > > -Ivan > > > > ping. > > ping. Hi, where are we at with this?
On 2015/12/03 11:58:21, mit wrote: > On 2015/10/29 10:24:40, floitsch wrote: > > On 2015/10/26 18:12:42, floitsch wrote: > > > On 2015/10/20 07:24:22, Ivan Posva wrote: > > > > Do NOT commit. I need to see the spec first before we can move forward on > > > this. > > > > > > > > Thanks, > > > > -Ivan > > > > > > ping. > > > > ping. > > Hi, where are we at with this? Can I commit this now?
-Ivan https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5851: String& key = String::ZoneHandle(Z); Why is this not a const String&? Also you do not need it as a ZoneHandle here either. https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5852: key = Symbols::FromConcatAll(pieces); The key does not need to be a Symbol and should only be allocated/concatenated if it is actually going to be used below in the lookup. https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5865: if (condition_triggered) continue; VM style is: if (expr) { statements; } https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5867: const String& value = valueNode == NULL (valueNode == NULL) https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5869: : String::Cast(valueNode->AsLiteralNode()->literal()); Please use the same assertions as below on line 5879 and 5880 to verify that we have the appropriate literal.
https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5851: String& key = String::ZoneHandle(Z); On 2015/12/17 06:29:43, Ivan Posva wrote: > Why is this not a const String&? Also you do not need it as a ZoneHandle here > either. Done. https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5852: key = Symbols::FromConcatAll(pieces); On 2015/12/17 06:29:43, Ivan Posva wrote: > The key does not need to be a Symbol and should only be allocated/concatenated > if it is actually going to be used below in the lookup. There isn't a good way to concatenate strings except by going through symbols. I copied a TODO from Srdjan (from report.cc) where the same pattern was used. Moved it below, so it's only computed, when needed. https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5865: if (condition_triggered) continue; On 2015/12/17 06:29:43, Ivan Posva wrote: > VM style is: > if (expr) { > statements; > } Done. https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5867: const String& value = valueNode == NULL On 2015/12/17 06:29:43, Ivan Posva wrote: > (valueNode == NULL) Done. https://codereview.chromium.org/1380383002/diff/80001/runtime/vm/parser.cc#ne... runtime/vm/parser.cc:5869: : String::Cast(valueNode->AsLiteralNode()->literal()); On 2015/12/17 06:29:43, Ivan Posva wrote: > Please use the same assertions as below on line 5879 and 5880 to verify that we > have the appropriate literal. Done.
https://codereview.chromium.org/1380383002/diff/140001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/1380383002/diff/140001/runtime/vm/parser.cc#n... runtime/vm/parser.cc:5980: const String& key = String::Handle(Symbols::FromConcatAll(pieces)); You should not be allocating a symbol here. A simple string will suffice, since the lifetime of that string is only during the lookup of the key in the environment. String::ConcatAll expects an Array handle, so you probably want to be collecting into a GrowableObjectArray first. For an example see BuildClosureSource in object.cc.
https://codereview.chromium.org/1380383002/diff/140001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/1380383002/diff/140001/runtime/vm/parser.cc#n... runtime/vm/parser.cc:5980: const String& key = String::Handle(Symbols::FromConcatAll(pieces)); On 2015/12/18 05:59:03, Ivan Posva wrote: > You should not be allocating a symbol here. A simple string will suffice, since > the lifetime of that string is only during the lookup of the key in the > environment. > > String::ConcatAll expects an Array handle, so you probably want to be collecting > into a GrowableObjectArray first. For an example see BuildClosureSource in > object.cc. Done.
LGTM -Ivan https://codereview.chromium.org/1380383002/diff/160001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/1380383002/diff/160001/runtime/vm/parser.cc#n... runtime/vm/parser.cc:5954: GrowableObjectArray::Handle(GrowableObjectArray::New()); Sorry missed this before in the reviews: *::Handle(T, ...) to avoid having the call the Thread::Current() in every handle allocation. https://codereview.chromium.org/1380383002/diff/160001/runtime/vm/parser.cc#n... runtime/vm/parser.cc:5986: String::Handle(Api::CallEnvironmentCallback(thread(), key)); thread() -> T
@Ivan, please confirm that "Z" is the right argument. (Almost certain, but prefer to wait). Thanks. https://codereview.chromium.org/1380383002/diff/160001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/1380383002/diff/160001/runtime/vm/parser.cc#n... runtime/vm/parser.cc:5954: GrowableObjectArray::Handle(GrowableObjectArray::New()); On 2015/12/18 18:15:39, Ivan Posva wrote: > Sorry missed this before in the reviews: > > *::Handle(T, ...) to avoid having the call the Thread::Current() in every handle > allocation. Do you mean "Z" ? If yes, done. https://codereview.chromium.org/1380383002/diff/160001/runtime/vm/parser.cc#n... runtime/vm/parser.cc:5986: String::Handle(Api::CallEnvironmentCallback(thread(), key)); On 2015/12/18 18:15:39, Ivan Posva wrote: > thread() -> T Done.
On 2015/12/19 06:36:04, floitsch wrote: > @Ivan, please confirm that "Z" is the right argument. (Almost certain, but > prefer to wait). > Thanks. > > https://codereview.chromium.org/1380383002/diff/160001/runtime/vm/parser.cc > File runtime/vm/parser.cc (right): > > https://codereview.chromium.org/1380383002/diff/160001/runtime/vm/parser.cc#n... > runtime/vm/parser.cc:5954: > GrowableObjectArray::Handle(GrowableObjectArray::New()); > On 2015/12/18 18:15:39, Ivan Posva wrote: > > Sorry missed this before in the reviews: > > > > *::Handle(T, ...) to avoid having the call the Thread::Current() in every > handle > > allocation. > > Do you mean "Z" ? > If yes, done. > > https://codereview.chromium.org/1380383002/diff/160001/runtime/vm/parser.cc#n... > runtime/vm/parser.cc:5986: String::Handle(Api::CallEnvironmentCallback(thread(), > key)); > On 2015/12/18 18:15:39, Ivan Posva wrote: > > thread() -> T > > Done. ping.
-Ivan https://codereview.chromium.org/1380383002/diff/160001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): https://codereview.chromium.org/1380383002/diff/160001/runtime/vm/parser.cc#n... runtime/vm/parser.cc:5954: GrowableObjectArray::Handle(GrowableObjectArray::New()); On 2015/12/19 06:36:03, floitsch wrote: > On 2015/12/18 18:15:39, Ivan Posva wrote: > > Sorry missed this before in the reviews: > > > > *::Handle(T, ...) to avoid having the call the Thread::Current() in every > handle > > allocation. > > Do you mean "Z" ? > If yes, done. Yes, Z is the correct thing to use here as it returns this->thread()->zone().
Description was changed from ========== Add support for configuration-specific imports to the VM. ========== to ========== Add support for configuration-specific imports to the VM. R=hausner@google.com, iposva@google.com Committed: https://github.com/dart-lang/sdk/commit/e0fe0c0ed87995150583579e4329f00f3e89f38e ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) manually as e0fe0c0ed87995150583579e4329f00f3e89f38e (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
