Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years ago by Clemens Hammacher Modified:
4 years ago Reviewers:
kozy, titzer, Yang CC:
devtools-reviews_chromium.org, jgruber, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[inspector] Split off interface-types.h
This CL adds a new header src/debug/interface-types.h, moves the
definition of Location from the debug-interface.h to this new header,
and adds a new definition for the WasmDisassembly types.
This allows to use the types in other implementation files or headers
without having to include the entire debug-interface.h, reducing build
dependencies and compile time (especially for incremental builds).
The WasmDisassembly type replaces the old
std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit
hard to unravel.
R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org
Committed: https://crrev.com/f5fb2da64cfd7adff84e6ebbfbaac24e3c47cdba
Cr-Commit-Position: refs/heads/master@{#41488}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Split off interface-types.h #Patch Set 3 : Add missing file #Patch Set 4 : Add missing namespace #Patch Set 5 : Come on, windows, be nice... #Patch Set 6 : Rebase #
Total comments: 2
Created: 4 years ago
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 57 (41 generated)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [wasm] Introduce type for disassembly result The old return type (pair<string, vector<tuple<...>>>) was a bit hard to unravel. By introducing a new struct in the DebugInterface, and assigning a name to the tuple type by a "using" declaration, the readability is improved significantly. ========== to ========== [wasm] Introduce type for disassembly result The old return type (pair<string, vector<tuple<...>>>) was a bit hard to unravel. By introducing a new struct in the DebugInterface, and assigning a name to the tuple type by a "using" declaration, the readability is improved significantly. R=yangguo@chromium.org, titzer@chromium.org ==========
clemensh@chromium.org changed reviewers: + yangguo@chromium.org
clemensh@chromium.org changed reviewers: + titzer@chromium.org
Yang, is this an acceptable solution to improve readability? It requires us to include the debug-interface.h in wasm code; currently it is only included from src/inspector and src/debug. We can also go a step further and introduce a new header which is included by both debug-interface.h and the wasm implementations. This way, we could completely get rid of the std::tuple. Would you prefer that? Where should the header file go then? Something like src/inspector/wasm-inspect.h?
kozyatinskiy@chromium.org changed reviewers: + kozyatinskiy@chromium.org
https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h File src/debug/debug-interface.h (right): https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h... src/debug/debug-interface.h:212: using offset_table_entry = std::tuple<uint32_t, int, int>; Could we replace this tuple with struct as well?
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17095) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/17149) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13453)
Description was changed from ========== [wasm] Introduce type for disassembly result The old return type (pair<string, vector<tuple<...>>>) was a bit hard to unravel. By introducing a new struct in the DebugInterface, and assigning a name to the tuple type by a "using" declaration, the readability is improved significantly. R=yangguo@chromium.org, titzer@chromium.org ========== to ========== [inspector] Split off interface-types.h This CL adds a new header src/debug/interface-types.h, moves the definition of Location from the debug-interface.h to this new header, and adds a new definition for the WasmDisassembly types. This allows to use the types in other implementation files or headers without having to include the entire debug-interface.h. The WasmDisassembly type replaces the old std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit hard to unravel. R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org ==========
Description was changed from ========== [inspector] Split off interface-types.h This CL adds a new header src/debug/interface-types.h, moves the definition of Location from the debug-interface.h to this new header, and adds a new definition for the WasmDisassembly types. This allows to use the types in other implementation files or headers without having to include the entire debug-interface.h. The WasmDisassembly type replaces the old std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit hard to unravel. R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org ========== to ========== [inspector] Split off interface-types.h This CL adds a new header src/debug/interface-types.h, moves the definition of Location from the debug-interface.h to this new header, and adds a new definition for the WasmDisassembly types. This allows to use the types in other implementation files or headers without having to include the entire debug-interface.h. The WasmDisassembly type replaces the old std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit hard to unravel. In the process of changing the wasm::DisassembleFunction function to use the new type, I also made it a method of the WasmCompiledModule class, where it belongs. R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org ==========
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/18669)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/29 at 23:52:45, kozyatinskiy wrote: > https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h... > src/debug/debug-interface.h:212: using offset_table_entry = std::tuple<uint32_t, int, int>; > Could we replace this tuple with struct as well? In order to achieve this, I now created a new header file src/debug/interface-types.h, where we can define such types. This avoids having to include the debug-interface.h header everywhere. I am now defining a new struct for the offset table entry, and the WasmDisassembly struct there, and use it directly where the data is created. I changed the description of this CL accordingly. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/18670)
On 2016/11/30 17:06:20, Clemens Hammacher wrote: > On 2016/11/29 at 23:52:45, kozyatinskiy wrote: > > > https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h... > > src/debug/debug-interface.h:212: using offset_table_entry = > std::tuple<uint32_t, int, int>; > > Could we replace this tuple with struct as well? > > In order to achieve this, I now created a new header file > src/debug/interface-types.h, where we can define such types. > This avoids having to include the debug-interface.h header everywhere. > I am now defining a new struct for the offset table entry, and the > WasmDisassembly struct there, and use it directly where the data is created. > > I changed the description of this CL accordingly. > PTAL. I like this approach. WASM stuff lgtm
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/30 17:06:20, Clemens Hammacher wrote: > On 2016/11/29 at 23:52:45, kozyatinskiy wrote: > > > https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h... > > src/debug/debug-interface.h:212: using offset_table_entry = > std::tuple<uint32_t, int, int>; > > Could we replace this tuple with struct as well? > > In order to achieve this, I now created a new header file > src/debug/interface-types.h, where we can define such types. > This avoids having to include the debug-interface.h header everywhere. > I am now defining a new struct for the offset table entry, and the > WasmDisassembly struct there, and use it directly where the data is created. > > I changed the description of this CL accordingly. > PTAL. I'm not sure that one more header file is needed. We use debug-interface.h as interface between V8 and inspector so we can put any types that is used on both sides in this file. Probably we can rename debug-interface in something more general. Deferring to Yang. rs inspector lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/30 at 17:28:15, kozyatinskiy wrote: > On 2016/11/30 17:06:20, Clemens Hammacher wrote: > > On 2016/11/29 at 23:52:45, kozyatinskiy wrote: > > > > > https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h... > > > src/debug/debug-interface.h:212: using offset_table_entry = > > std::tuple<uint32_t, int, int>; > > > Could we replace this tuple with struct as well? > > > > In order to achieve this, I now created a new header file > > src/debug/interface-types.h, where we can define such types. > > This avoids having to include the debug-interface.h header everywhere. > > I am now defining a new struct for the offset table entry, and the > > WasmDisassembly struct there, and use it directly where the data is created. > > > > I changed the description of this CL accordingly. > > PTAL. > > I'm not sure that one more header file is needed. We use debug-interface.h as interface between V8 and inspector so we can put any types that is used on both sides in this file. Probably we can rename debug-interface in something more general. > Deferring to Yang. > > rs inspector lgtm. There is another problem with the types defined inside of the DebugInterface class: You can not forward-declare them. Solving this does not require a separate header though. The main idea of the separate header is to reduce build dependencies and compile time.
On 2016/11/30 17:46:05, Clemens Hammacher wrote: > On 2016/11/30 at 17:28:15, kozyatinskiy wrote: > > On 2016/11/30 17:06:20, Clemens Hammacher wrote: > > > On 2016/11/29 at 23:52:45, kozyatinskiy wrote: > > > > > > > > https://codereview.chromium.org/2529383002/diff/1/src/debug/debug-interface.h... > > > > src/debug/debug-interface.h:212: using offset_table_entry = > > > std::tuple<uint32_t, int, int>; > > > > Could we replace this tuple with struct as well? > > > > > > In order to achieve this, I now created a new header file > > > src/debug/interface-types.h, where we can define such types. > > > This avoids having to include the debug-interface.h header everywhere. > > > I am now defining a new struct for the offset table entry, and the > > > WasmDisassembly struct there, and use it directly where the data is created. > > > > > > I changed the description of this CL accordingly. > > > PTAL. > > > > I'm not sure that one more header file is needed. We use debug-interface.h as > interface between V8 and inspector so we can put any types that is used on both > sides in this file. Probably we can rename debug-interface in something more > general. > > Deferring to Yang. > > > > rs inspector lgtm. > > There is another problem with the types defined inside of the DebugInterface > class: You can not forward-declare them. Solving this does not require a > separate header though. > The main idea of the separate header is to reduce build dependencies and compile > time. Yes. I like the separate header for exactly these reasons.
On 2016/11/30 17:46:05, Clemens Hammacher wrote: > There is another problem with the types defined inside of the DebugInterface > class: You can not forward-declare them. Solving this does not require a > separate header though. Let's replace class DebugInterface with namespace DebugInterface. > The main idea of the separate header is to reduce build dependencies and compile > time. I see, what is real difference in compile time before and after split into two headers? I think we can split it later if it would be real performance issue.
Description was changed from ========== [inspector] Split off interface-types.h This CL adds a new header src/debug/interface-types.h, moves the definition of Location from the debug-interface.h to this new header, and adds a new definition for the WasmDisassembly types. This allows to use the types in other implementation files or headers without having to include the entire debug-interface.h. The WasmDisassembly type replaces the old std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit hard to unravel. In the process of changing the wasm::DisassembleFunction function to use the new type, I also made it a method of the WasmCompiledModule class, where it belongs. R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org ========== to ========== [inspector] Split off interface-types.h This CL adds a new header src/debug/interface-types.h, moves the definition of Location from the debug-interface.h to this new header, and adds a new definition for the WasmDisassembly types. This allows to use the types in other implementation files or headers without having to include the entire debug-interface.h, reducing build dependencies and compile time (especially for incremental builds). The WasmDisassembly type replaces the old std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit hard to unravel. R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org ==========
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/30 18:02:53, kozyatinskiy wrote: > On 2016/11/30 17:46:05, Clemens Hammacher wrote: > > There is another problem with the types defined inside of the DebugInterface > > class: You can not forward-declare them. Solving this does not require a > > separate header though. > > Let's replace class DebugInterface with namespace DebugInterface. > > > The main idea of the separate header is to reduce build dependencies and > compile > > time. > > I see, what is real difference in compile time before and after split into two > headers? I think we can split it later if it would be real performance issue. It is admittedly small, but since it's already done (and I think it's cleaner), maybe we should just go ahead and land this one.
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org, kozyatinskiy@chromium.org Link to the patchset: https://codereview.chromium.org/2529383002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/29866)
LGTM with comment https://codereview.chromium.org/2529383002/diff/100001/src/debug/interface-ty... File src/debug/interface-types.h (right): https://codereview.chromium.org/2529383002/diff/100001/src/debug/interface-ty... src/debug/interface-types.h:13: namespace debug { Having Location in the namespace v8::debug, and DebugInterface in the namespace v8 is somewhat weird. Can we have a follow-up CL that moves everything in DebugInterface into v8::debug as well?
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org
https://codereview.chromium.org/2529383002/diff/100001/src/debug/interface-ty... File src/debug/interface-types.h (right): https://codereview.chromium.org/2529383002/diff/100001/src/debug/interface-ty... src/debug/interface-types.h:13: namespace debug { On 2016/12/05 at 12:44:49, Yang wrote: > Having Location in the namespace v8::debug, and DebugInterface in the namespace v8 is somewhat weird. > > Can we have a follow-up CL that moves everything in DebugInterface into v8::debug as well? It's here: http://crrev.com/2549133002 I don't particularly like it, since you now often have to write debug::DebugInterface or even v8::debug::DebugInterface. Thanks for supporting an incremental process here!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480944604147010,
"parent_rev": "fc14a87a8b2b02d77ae994130b423b7056072216", "commit_rev":
"cdc9f0782769288310bb071c58688a1c61cee5fc"}
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] Split off interface-types.h This CL adds a new header src/debug/interface-types.h, moves the definition of Location from the debug-interface.h to this new header, and adds a new definition for the WasmDisassembly types. This allows to use the types in other implementation files or headers without having to include the entire debug-interface.h, reducing build dependencies and compile time (especially for incremental builds). The WasmDisassembly type replaces the old std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit hard to unravel. R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org ========== to ========== [inspector] Split off interface-types.h This CL adds a new header src/debug/interface-types.h, moves the definition of Location from the debug-interface.h to this new header, and adds a new definition for the WasmDisassembly types. This allows to use the types in other implementation files or headers without having to include the entire debug-interface.h, reducing build dependencies and compile time (especially for incremental builds). The WasmDisassembly type replaces the old std::pair<std::string, std::vector<std::tuple<...>>>, which was a bit hard to unravel. R=yangguo@chromium.org, kozyatinskiy@chromium.org, titzer@chromium.org Committed: https://crrev.com/f5fb2da64cfd7adff84e6ebbfbaac24e3c47cdba Cr-Commit-Position: refs/heads/master@{#41488} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f5fb2da64cfd7adff84e6ebbfbaac24e3c47cdba Cr-Commit-Position: refs/heads/master@{#41488} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
