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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1264993002: Debugger: refactor ScopeIterator, FrameInspector and DebugEvaluate. (Closed)

Created:
5 years, 4 months ago by Yang
Modified:
5 years, 4 months ago
Reviewers:
ulan, brucedawson
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

Debugger: refactor ScopeIterator, FrameInspector and DebugEvaluate. This is a pure refactor and does not change functionality. R=ulan@chromium.org Committed: https://crrev.com/4e036f3042257f83e5c0d0ce6604a0dfeeb77643 Cr-Commit-Position: refs/heads/master@{#29995}

Patch Set 1 #

Patch Set 2 : remove some includes #

Patch Set 3 : also DebugEvaluate #

Patch Set 4 : readd include #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+1654 lines, -1431 lines) Patch
M BUILD.gn View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
A src/debug/debug-evaluate.h View 1 2 1 chunk +100 lines, -0 lines 0 comments Download
A src/debug/debug-evaluate.cc View 1 2 1 chunk +319 lines, -0 lines 0 comments Download
A src/debug/debug-frames.h View 1 2 1 chunk +81 lines, -0 lines 0 comments Download
A src/debug/debug-frames.cc View 1 2 1 chunk +217 lines, -0 lines 0 comments Download
A src/debug/debug-scopes.h View 1 1 chunk +126 lines, -0 lines 0 comments Download
A src/debug/debug-scopes.cc View 1 1 chunk +769 lines, -0 lines 2 comments Download
M src/globals.h View 1 chunk +0 lines, -1 line 0 comments Download
M src/isolate.h View 1 chunk +0 lines, -1 line 0 comments Download
M src/objects.h View 2 chunks +2 lines, -6 lines 0 comments Download
M src/runtime/runtime.h View 1 2 1 chunk +0 lines, -2 lines 0 comments Download
M src/runtime/runtime-debug.cc View 1 2 3 20 chunks +25 lines, -1395 lines 0 comments Download
M src/runtime/runtime-liveedit.cc View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M src/scopeinfo.cc View 1 chunk +0 lines, -24 lines 0 comments Download
M test/cctest/test-debug.cc View 1 chunk +0 lines, -1 line 0 comments Download
M tools/gyp/v8.gyp View 1 2 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 28 (10 generated)
Yang
5 years, 4 months ago (2015-07-31 13:39:32 UTC) #1
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264993002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264993002/1
5 years, 4 months ago (2015-07-31 13:46:58 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264993002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264993002/20001
5 years, 4 months ago (2015-07-31 16:38:05 UTC) #5
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 4 months ago (2015-07-31 17:02:41 UTC) #7
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264993002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264993002/40001
5 years, 4 months ago (2015-08-03 11:05:33 UTC) #9
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 4 months ago (2015-08-03 11:36:38 UTC) #11
Yang
On 2015/08/03 11:36:38, commit-bot: I haz the power wrote: > Dry run: This issue passed ...
5 years, 4 months ago (2015-08-04 05:57:29 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264993002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264993002/60001
5 years, 4 months ago (2015-08-04 08:27:34 UTC) #15
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 4 months ago (2015-08-04 08:55:04 UTC) #17
ulan
Are the function bodies copied verbatim without any changes?
5 years, 4 months ago (2015-08-04 11:11:29 UTC) #18
Yang
On 2015/08/04 11:11:29, ulan wrote: > Are the function bodies copied verbatim without any changes? ...
5 years, 4 months ago (2015-08-04 11:14:11 UTC) #19
ulan
rubber-stamp lgtm
5 years, 4 months ago (2015-08-04 11:20:52 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264993002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264993002/60001
5 years, 4 months ago (2015-08-04 12:07:47 UTC) #22
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 4 months ago (2015-08-04 12:09:21 UTC) #23
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/4e036f3042257f83e5c0d0ce6604a0dfeeb77643 Cr-Commit-Position: refs/heads/master@{#29995}
5 years, 4 months ago (2015-08-04 12:10:22 UTC) #24
brucedawson
https://codereview.chromium.org/1264993002/diff/60001/src/debug/debug-scopes.cc File src/debug/debug-scopes.cc (right): https://codereview.chromium.org/1264993002/diff/60001/src/debug/debug-scopes.cc#newcode86 src/debug/debug-scopes.cc:86: Handle<ScopeInfo> scope_info(shared_info->scope_info()); This variable shadows the same-named variable in ...
5 years, 4 months ago (2015-08-10 18:08:08 UTC) #26
Yang
https://codereview.chromium.org/1264993002/diff/60001/src/debug/debug-scopes.cc File src/debug/debug-scopes.cc (right): https://codereview.chromium.org/1264993002/diff/60001/src/debug/debug-scopes.cc#newcode86 src/debug/debug-scopes.cc:86: Handle<ScopeInfo> scope_info(shared_info->scope_info()); On 2015/08/10 18:08:08, brucedawson wrote: > This ...
5 years, 4 months ago (2015-08-11 08:13:36 UTC) #27
Yang
5 years, 4 months ago (2015-08-11 08:14:53 UTC) #28
Message was sent while issue was closed. 
On 2015/08/11 08:13:36, Yang wrote:
>
https://codereview.chromium.org/1264993002/diff/60001/src/debug/debug-scopes.cc
> File src/debug/debug-scopes.cc (right):
> 
>
https://codereview.chromium.org/1264993002/diff/60001/src/debug/debug-scopes....
> src/debug/debug-scopes.cc:86: Handle<ScopeInfo>
> scope_info(shared_info->scope_info());
> On 2015/08/10 18:08:08, brucedawson wrote:
> > This variable shadows the same-named variable in the outer scope declared at
> > line 33. This is probably harmless but perhaps (I really don't know)
> inefficient
> > since they seem to be recording the same information.
> > 
> > FYI.
> > 
> > This was noticed as a new /analyze warning:
> > debug-scopes.cc(86) : warning C6246: Local declaration of 'scope_info' hides
> > declaration of the same name in outer scope. See previous declaration at
line
> > '33'.
> > 
> > Fixing is not required.
> 
> You are right. This is indeed redundant. I simply copied over old code and did
> not notice. I'll follow up with a new CL.

Said follow-up: https://codereview.chromium.org/1286653004

Powered by Google App Engine
This is Rietveld 408576698