Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10134015:
added package_root flag to standalone dart vm (Closed)
|
Created:
8 years, 8 months ago by mattsh Modified:
8 years, 8 months ago CC:
reviews_dartlang.org Visibility:
Public. |
Descriptionadded package_root flag to vm
BUG=
TEST=
Committed: https://code.google.com/p/dart/source/detail?r=7092
Patch Set 1 #Patch Set 2 : small tweak #
Total comments: 15
Patch Set 3 : code review fixes #Patch Set 4 : add error handling #
Total comments: 6
Patch Set 5 : code review blank line fixes #Patch Set 6 : code review fixes #
Total comments: 16
Patch Set 7 : code review fixes #
Total comments: 2
Patch Set 8 : fix blank lines #Messages
Total messages: 15 (0 generated)
One nit but LGTM. Obviously wait for Siva to give the real LGTM, though. :) https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/builtin... File runtime/bin/builtin.dart (right): https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/builtin... runtime/bin/builtin.dart:154: String path; Just use "var" here to be consistent with the rest of the file.
https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/builtin... File runtime/bin/builtin.dart (right): https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/builtin... runtime/bin/builtin.dart:49: _packageRoot = packageRoot; will windows users specify --package_root="c:\Users\billg\" or --package_root="c:/Users/billg/" ? The later one is not natural and if it is the former than the code you have in builtin.dart for setting the package root may not work. Also what happens if they specify "--package_root=c:" meaning current directory in drive c: ? https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc File runtime/bin/main.cc (right): https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc... runtime/bin/main.cc:52: static const char* package_root; need to initialize this to NULL otherwise your check below of package_root == NULL will succeed or fail randomly. https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc... runtime/bin/main.cc:415: dart_args[0] = Dart_NewString(package_root); Dart_NewString can return errors and that needs to be checked. Agreed there are lots of places in this file which does not do this we will fix those in a different CL. https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc... runtime/bin/main.cc:416: Dart_Invoke(builtin_lib, Dart_NewString("_setPackageRoot"), 1, dart_args); The return value from Dart_NewString and Dart_Invoke should be checked using Dart_IsError.
https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/builtin... File runtime/bin/builtin.dart (right): https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/builtin... runtime/bin/builtin.dart:49: _packageRoot = packageRoot; On 2012/04/22 00:14:01, asiva wrote: > will windows users specify --package_root="c:\Users\billg\" or > --package_root="c:/Users/billg/" ? > > The later one is not natural and if it is the former than the code you have in > builtin.dart for setting the package root may not work. > > > Also what happens if they specify "--package_root=c:" meaning current directory > in drive c: ? Adding a TODO to decide about forward slashes and drive letters. https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc File runtime/bin/main.cc (right): https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc... runtime/bin/main.cc:52: static const char* package_root; On 2012/04/22 00:14:01, asiva wrote: > need to initialize this to NULL otherwise your check below of > package_root == NULL will succeed or fail randomly. Actually C/C++ guarantee to initialize statics to zero automatically, so it's OK, but I added the = NULL now for clarity. https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc... runtime/bin/main.cc:415: dart_args[0] = Dart_NewString(package_root); On 2012/04/22 00:14:01, asiva wrote: > Dart_NewString can return errors and that needs to be checked. > Agreed there are lots of places in this file which does not do this we will fix > those in a different CL. Added a TODO https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc... runtime/bin/main.cc:416: Dart_Invoke(builtin_lib, Dart_NewString("_setPackageRoot"), 1, dart_args); On 2012/04/22 00:14:01, asiva wrote: > The return value from Dart_NewString and Dart_Invoke should be checked using > Dart_IsError. Added a TODO to add the error handling.
https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/builtin... File runtime/bin/builtin.dart (right): https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/builtin... runtime/bin/builtin.dart:49: _packageRoot = packageRoot; We would like to not postpone windows issues for a later date as we seem to have numerous windows users lately who have been complaining if things don't work. On 2012/04/23 16:48:09, mattsh wrote: > On 2012/04/22 00:14:01, asiva wrote: > > will windows users specify --package_root="c:\Users\billg\" or > > --package_root="c:/Users/billg/" ? > > > > The later one is not natural and if it is the former than the code you have in > > builtin.dart for setting the package root may not work. > > > > > > Also what happens if they specify "--package_root=c:" meaning current > directory > > in drive c: ? > > Adding a TODO to decide about forward slashes and drive letters. https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc File runtime/bin/main.cc (right): https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc... runtime/bin/main.cc:415: dart_args[0] = Dart_NewString(package_root); Not sure why this needs to be a TODO and not fixed now. On 2012/04/23 16:48:09, mattsh wrote: > On 2012/04/22 00:14:01, asiva wrote: > > Dart_NewString can return errors and that needs to be checked. > > Agreed there are lots of places in this file which does not do this we will > fix > > those in a different CL. > > Added a TODO https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc... runtime/bin/main.cc:416: Dart_Invoke(builtin_lib, Dart_NewString("_setPackageRoot"), 1, dart_args); Ditto comment about why this needs to be a TODO. On 2012/04/23 16:48:09, mattsh wrote: > On 2012/04/22 00:14:01, asiva wrote: > > The return value from Dart_NewString and Dart_Invoke should be checked using > > Dart_IsError. > > Added a TODO to add the error handling.
https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc File runtime/bin/main.cc (right): https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc... runtime/bin/main.cc:415: dart_args[0] = Dart_NewString(package_root); On 2012/04/23 17:16:45, asiva wrote: > Not sure why this needs to be a TODO and not fixed now. OK, now added the error handling here and removed the TODO. > > On 2012/04/23 16:48:09, mattsh wrote: > > On 2012/04/22 00:14:01, asiva wrote: > > > Dart_NewString can return errors and that needs to be checked. > > > Agreed there are lots of places in this file which does not do this we will > > fix > > > those in a different CL. > > > > Added a TODO > https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc... runtime/bin/main.cc:416: Dart_Invoke(builtin_lib, Dart_NewString("_setPackageRoot"), 1, dart_args); On 2012/04/22 00:14:01, asiva wrote: > The return value from Dart_NewString and Dart_Invoke should be checked using > Dart_IsError. Done. https://chromiumcodereview.appspot.com/10134015/diff/2001/runtime/bin/main.cc... runtime/bin/main.cc:416: Dart_Invoke(builtin_lib, Dart_NewString("_setPackageRoot"), 1, dart_args); On 2012/04/23 17:16:45, asiva wrote: > Ditto comment about why this needs to be a TODO. > > On 2012/04/23 16:48:09, mattsh wrote: > > On 2012/04/22 00:14:01, asiva wrote: > > > The return value from Dart_NewString and Dart_Invoke should be checked using > > > Dart_IsError. > > > > Added a TODO to add the error handling. > Done.
A couple of quick things I noticed while scrolling through this change. -Ivan https://chromiumcodereview.appspot.com/10134015/diff/13001/runtime/bin/main.cc File runtime/bin/main.cc (left): https://chromiumcodereview.appspot.com/10134015/diff/13001/runtime/bin/main.c... runtime/bin/main.cc:85: Why this removed line? https://chromiumcodereview.appspot.com/10134015/diff/13001/runtime/bin/main.cc File runtime/bin/main.cc (right): https://chromiumcodereview.appspot.com/10134015/diff/13001/runtime/bin/main.c... runtime/bin/main.cc:78: Two lines, as in the rest of the file, please. https://chromiumcodereview.appspot.com/10134015/diff/13001/runtime/bin/main.c... runtime/bin/main.cc:379: // returns true on success, false on failure Comments end with . and start with a capital. Make them real sentences.
https://chromiumcodereview.appspot.com/10134015/diff/13001/runtime/bin/main.cc File runtime/bin/main.cc (left): https://chromiumcodereview.appspot.com/10134015/diff/13001/runtime/bin/main.c... runtime/bin/main.cc:85: On 2012/04/25 16:25:19, Ivan Posva wrote: > Why this removed line? Done. https://chromiumcodereview.appspot.com/10134015/diff/13001/runtime/bin/main.cc File runtime/bin/main.cc (right): https://chromiumcodereview.appspot.com/10134015/diff/13001/runtime/bin/main.c... runtime/bin/main.cc:78: On 2012/04/25 16:25:19, Ivan Posva wrote: > Two lines, as in the rest of the file, please. Done. https://chromiumcodereview.appspot.com/10134015/diff/13001/runtime/bin/main.c... runtime/bin/main.cc:379: // returns true on success, false on failure On 2012/04/25 16:25:19, Ivan Posva wrote: > Comments end with . and start with a capital. Make them real sentences. Done.
LGTM once comments are addressed. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... File runtime/bin/builtin.dart (right): https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... runtime/bin/builtin.dart:49: packageRoot += "/"; The '+' operator has been deprecated and is being removed from the VM code base. Would be preferable if we did not use it in any new dart code additions. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... runtime/bin/builtin.dart:158: path = _packageRoot + uri.path; Ditto comment regarding '+' operator. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/main.cc File runtime/bin/main.cc (right): https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/main.c... runtime/bin/main.cc:380: Missing blank line. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/main.c... runtime/bin/main.cc:426: if (Dart_IsError(result)) { *error = strdup(Dart_GetError(result));
https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... File runtime/bin/builtin.dart (right): https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... runtime/bin/builtin.dart:47: // support backslash. If you check a couple of lines below, there is example code how to deal with back slashes on Windows. Only supporting forward slashes is a restriction that we do not have in other areas of the system and it is inconsistent and surprising to have this restriction here. Please fix. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... runtime/bin/builtin.dart:156: String path; No types for locals. Please preserve the style of this file. Thank you. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/main.cc File runtime/bin/main.cc (right): https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/main.c... runtime/bin/main.cc:97: Two lines. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/main.c... runtime/bin/main.cc:420: return false; This place is also missing the setup of the error result. See below.
https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... File runtime/bin/builtin.dart (right): https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... runtime/bin/builtin.dart:47: // support backslash. It is not just the backslash that is a problem. Consider somebody specifying --package_root="d:" meaning they want to use the current directory on drive d:, a perfectly normal operation on windows. In that case we are not supposed to stick a backslash in. On 2012/04/26 05:32:09, Ivan Posva wrote: > If you check a couple of lines below, there is example code how to deal with > back slashes on Windows. Only supporting forward slashes is a restriction that > we do not have in other areas of the system and it is inconsistent and > surprising to have this restriction here. Please fix.
https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... File runtime/bin/builtin.dart (right): https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... runtime/bin/builtin.dart:47: // support backslash. On 2012/04/26 05:32:09, Ivan Posva wrote: > If you check a couple of lines below, there is example code how to deal with > back slashes on Windows. Only supporting forward slashes is a restriction that > we do not have in other areas of the system and it is inconsistent and > surprising to have this restriction here. Please fix. If it's OK, I'll put in a TODO now, because I think we want to rework the other code slightly that handles windows so it can be reused here, but that should be a separate CL. (I.e., we don't want to have two places of code that do the backslash replacement.) https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... runtime/bin/builtin.dart:49: packageRoot += "/"; On 2012/04/25 23:26:12, asiva wrote: > The '+' operator has been deprecated and is being removed from the VM code base. > Would be preferable if we did not use it in any new dart code additions. Done. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... runtime/bin/builtin.dart:156: String path; On 2012/04/26 05:32:09, Ivan Posva wrote: > No types for locals. Please preserve the style of this file. Thank you. Done. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/builti... runtime/bin/builtin.dart:158: path = _packageRoot + uri.path; On 2012/04/25 23:26:12, asiva wrote: > Ditto comment regarding '+' operator. Done. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/main.cc File runtime/bin/main.cc (right): https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/main.c... runtime/bin/main.cc:380: On 2012/04/25 23:26:12, asiva wrote: > Missing blank line. Done. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/main.c... runtime/bin/main.cc:420: return false; On 2012/04/26 05:32:09, Ivan Posva wrote: > This place is also missing the setup of the error result. See below. Done. https://chromiumcodereview.appspot.com/10134015/diff/19001/runtime/bin/main.c... runtime/bin/main.cc:426: if (Dart_IsError(result)) { On 2012/04/25 23:26:12, asiva wrote: > *error = strdup(Dart_GetError(result)); Done.
LGTM https://chromiumcodereview.appspot.com/10134015/diff/10003/runtime/bin/main.cc File runtime/bin/main.cc (right): https://chromiumcodereview.appspot.com/10134015/diff/10003/runtime/bin/main.c... runtime/bin/main.cc:381: Missing blank line (2 blank lines between functions).
https://chromiumcodereview.appspot.com/10134015/diff/10003/runtime/bin/main.cc File runtime/bin/main.cc (right): https://chromiumcodereview.appspot.com/10134015/diff/10003/runtime/bin/main.c... runtime/bin/main.cc:381: On 2012/04/27 17:07:46, asiva wrote: > Missing blank line (2 blank lines between functions). Done. |
