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

Issue 10703150: Don't create a Bigint object first for every integer while parsing the sources, instead first check… (Closed)

Created:
8 years, 5 months ago by siva
Modified:
8 years, 5 months ago
Reviewers:
sra1, cshapiro, Ivan Posva
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Visibility:
Public.

Description

Don't create a Bigint object first for every integer while parsing the sources, instead first check to see if it can be represented as a Smi or Mint. This avoids unnecessary creation of Bigint objects. Committed: https://code.google.com/p/dart/source/detail?r=9607

Patch Set 1 #

Total comments: 10

Patch Set 2 : #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+87 lines, -8 lines) Patch
M vm/object.cc View 1 1 chunk +10 lines, -8 lines 0 comments Download
M vm/os.h View 1 1 chunk +8 lines, -0 lines 0 comments Download
M vm/os_linux.cc View 1 1 chunk +23 lines, -0 lines 2 comments Download
M vm/os_macos.cc View 1 1 chunk +23 lines, -0 lines 0 comments Download
M vm/os_win.cc View 1 1 chunk +23 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
siva
8 years, 5 months ago (2012-07-11 20:57:15 UTC) #1
Ivan Posva
LGTM -ip
8 years, 5 months ago (2012-07-11 21:07:57 UTC) #2
sra1
DBC https://chromiumcodereview.appspot.com/10703150/diff/1/vm/os.h File vm/os.h (right): https://chromiumcodereview.appspot.com/10703150/diff/1/vm/os.h#newcode89 vm/os.h:89: // value. Please document the valid inputs. E.g., ...
8 years, 5 months ago (2012-07-11 21:46:43 UTC) #3
cshapiro
https://chromiumcodereview.appspot.com/10703150/diff/1/vm/object.cc File vm/object.cc (right): https://chromiumcodereview.appspot.com/10703150/diff/1/vm/object.cc#newcode7693 vm/object.cc:7693: // We are not supposed to have integers represented ...
8 years, 5 months ago (2012-07-11 22:11:02 UTC) #4
siva
https://chromiumcodereview.appspot.com/10703150/diff/1/vm/object.cc File vm/object.cc (right): https://chromiumcodereview.appspot.com/10703150/diff/1/vm/object.cc#newcode7693 vm/object.cc:7693: // We are not supposed to have integers represented ...
8 years, 5 months ago (2012-07-12 18:28:22 UTC) #5
sra1
https://chromiumcodereview.appspot.com/10703150/diff/1/vm/os_linux.cc File vm/os_linux.cc (right): https://chromiumcodereview.appspot.com/10703150/diff/1/vm/os_linux.cc#newcode177 vm/os_linux.cc:177: *value = strtoll(str, NULL, base); On 2012/07/12 18:28:23, asiva ...
8 years, 5 months ago (2012-07-12 20:12:14 UTC) #6
siva
8 years, 5 months ago (2012-07-16 19:45:47 UTC) #7
https://chromiumcodereview.appspot.com/10703150/diff/1/vm/os_linux.cc
File vm/os_linux.cc (right):

https://chromiumcodereview.appspot.com/10703150/diff/1/vm/os_linux.cc#newcode177
vm/os_linux.cc:177: *value = strtoll(str, NULL, base);
Good point, I have addressed this in a different CL.

On 2012/07/12 20:12:15, sra1 wrote:
> On 2012/07/12 18:28:23, asiva wrote:
> > I am not sure I understand the value of this additional check, it is
possible
> > for 
> > (endptr != str && *endptr == 0)
> > to be true and errno could still be ERANGE
> > 
> > We are not interested here in reporting why the function failed etc.
> > 
> > On 2012/07/11 21:46:43, sra1 wrote:
> > > You should pass in &endptr and verify endptr != str && *endptr == 0.
> > 
> 
> You don't get ERANGE or EINVAL for trailing junk.
> The only way to detect lack of trailing junk is to check that *endptr == 0.
> 
> const char *str = '123X';
> int base = 10;
> ...
> const char *endptr;
> int64_t value = stroll('123X', &endptr, base);
> if (endptr == str || *endptr != 0) return false;  // necessary check.

https://chromiumcodereview.appspot.com/10703150/diff/2003/vm/os_linux.cc
File vm/os_linux.cc (right):

https://chromiumcodereview.appspot.com/10703150/diff/2003/vm/os_linux.cc#newc...
vm/os_linux.cc:176: *value = strtoll(str, NULL, base);
Addressed this in a new CL.

On 2012/07/12 20:12:15, sra1 wrote:
> There is a bug here with the minimum value.
> Since you skipped the minus sign you are asking strtoll to parse the max value
+
> 1 so it will overflow.
> Since strtoll accepts a minus sign, you can fix this my passing the original
str
> parameter value.

Powered by Google App Engine
This is Rietveld 408576698