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

Issue 130443002: Properly parse V8 lines involving eval in pkg/stack_trace. (Closed)

Created:
6 years, 11 months ago by nweiz
Modified:
6 years, 11 months ago
CC:
reviews_dartlang.org, Bob Nystrom
Visibility:
Public.

Description

Properly parse V8 lines involving eval in pkg/stack_trace. R=sigmund@google.com Committed: https://code.google.com/p/dart/source/detail?r=31644

Patch Set 1 #

Total comments: 2

Patch Set 2 : code review #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -15 lines) Patch
M pkg/stack_trace/lib/src/frame.dart View 1 2 chunks +41 lines, -15 lines 4 comments Download
M pkg/stack_trace/test/frame_test.dart View 1 chunk +30 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
nweiz
6 years, 11 months ago (2014-01-09 01:44:16 UTC) #1
Siggi Cherem (dart-lang)
lgtm https://codereview.chromium.org/130443002/diff/1/pkg/stack_trace/lib/src/frame.dart File pkg/stack_trace/lib/src/frame.dart (right): https://codereview.chromium.org/130443002/diff/1/pkg/stack_trace/lib/src/frame.dart#newcode176 pkg/stack_trace/lib/src/frame.dart:176: return parseLocation(match[2], match[1].replaceAll("<anonymous>", "<fn>")); 80 col
6 years, 11 months ago (2014-01-09 01:47:53 UTC) #2
nweiz
https://codereview.chromium.org/130443002/diff/1/pkg/stack_trace/lib/src/frame.dart File pkg/stack_trace/lib/src/frame.dart (right): https://codereview.chromium.org/130443002/diff/1/pkg/stack_trace/lib/src/frame.dart#newcode176 pkg/stack_trace/lib/src/frame.dart:176: return parseLocation(match[2], match[1].replaceAll("<anonymous>", "<fn>")); On 2014/01/09 01:47:53, Siggi Cherem ...
6 years, 11 months ago (2014-01-09 01:52:20 UTC) #3
nweiz
Committed patchset #2 manually as r31644 (presubmit successful).
6 years, 11 months ago (2014-01-09 01:52:37 UTC) #4
Bob Nystrom
Couple of suggestions but LGTM. https://codereview.chromium.org/130443002/diff/40001/pkg/stack_trace/lib/src/frame.dart File pkg/stack_trace/lib/src/frame.dart (right): https://codereview.chromium.org/130443002/diff/40001/pkg/stack_trace/lib/src/frame.dart#newcode21 pkg/stack_trace/lib/src/frame.dart:21: r'^\s*at (?:([^\s].*?)(?: \[as [^\]]+\])? ...
6 years, 11 months ago (2014-01-09 17:22:00 UTC) #5
nweiz
6 years, 11 months ago (2014-01-09 21:12:34 UTC) #6
Message was sent while issue was closed.
https://codereview.chromium.org/130443002/diff/40001/pkg/stack_trace/lib/src/...
File pkg/stack_trace/lib/src/frame.dart (right):

https://codereview.chromium.org/130443002/diff/40001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/frame.dart:21: r'^\s*at (?:([^\s].*?)(?: \[as [^\]]+\])?
\((.*)\)|(.*))$');
On 2014/01/09 17:22:00, Bob Nystrom wrote:
> You can use "\S" instead of "[^\s]", I think. Here and elsewhere.

Done.

> Also, is the "." after "[^\s]" intentional? Did you mean to do "[^\s]*?"?

Yes; it's supposed to be an arbitrary string that starts with something other
than whitespace.

https://codereview.chromium.org/130443002/diff/40001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/frame.dart:157: if (evalMatch != null) return
parseLocation(evalMatch[1], member);
On 2014/01/09 17:22:00, Bob Nystrom wrote:
> I'd prefer iteration over recursion here.

Done.

Powered by Google App Engine
This is Rietveld 408576698