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

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

Reviewers:
Bob Nystrom, Siggi Cherem (dart-lang)

CC:
reviews_dartlang.org, Bob Nystrom

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

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 2 : code review #

Total comments: 4

Created: 6 years, 11 months ago

Download [raw] [tar.bz2]

		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)

Expand Messages | Collapse Messages

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.

Expand Messages | Collapse Messages