|
|
Created:
4 years ago by brucedawson Modified:
3 years, 11 months ago Reviewers:
stanisc CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionScript to compare global variables between two PDBs
This script relies on having the recently added ShowGlobals.exe built
and in your path.
R=stanisc@chromium.org
BUG=630755
Committed: https://crrev.com/cc618746e94bb7503d926317abe6d9c37531d35e
Cr-Commit-Position: refs/heads/master@{#440764}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Code review changes #Messages
Total messages: 14 (5 generated)
lgtm with a few nits https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... File tools/win/pdb_compare_globals.py (right): https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... tools/win/pdb_compare_globals.py:47: def ShowExtras(pdbA, pdbB, nameA, nameB): pdbA and pdbB names are a bit confusing because these are actually collections of data extracted from pdb's. It took me some time to realize that. https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... tools/win/pdb_compare_globals.py:60: if key in pdbB: if key in pdbA and key in pdbB ? https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... tools/win/pdb_compare_globals.py:68: print '\t' + '\t'.join(pdbB[key]) replace pdbA[key] with value_a and pdbB[key] with value_b
https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... File tools/win/pdb_compare_globals.py (right): https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... tools/win/pdb_compare_globals.py:47: def ShowExtras(pdbA, pdbB, nameA, nameB): On 2016/12/22 21:13:20, stanisc wrote: > pdbA and pdbB names are a bit confusing because these are actually collections > of data extracted from pdb's. It took me some time to realize that. How about symbols_1/symbols_2 and symbols_A/symbols_B? symbols_left, symbols_right? I'm leaning towards symbols_1/symbols_2 in main and symbols_A/symbols_B in the other functions. I could use the same names in main() and in the other functions but I'm worried that would be confusing, so I need two sets of names, I think. Any suggestions? https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... tools/win/pdb_compare_globals.py:60: if key in pdbB: On 2016/12/22 21:13:20, stanisc wrote: > if key in pdbA and key in pdbB ? The key is guaranteed to be in pdbA because we are iterating through pdbA. https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... tools/win/pdb_compare_globals.py:68: print '\t' + '\t'.join(pdbB[key]) On 2016/12/22 21:13:20, stanisc wrote: > replace pdbA[key] with value_a and pdbB[key] with value_b Good catch. Done.
https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... File tools/win/pdb_compare_globals.py (right): https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... tools/win/pdb_compare_globals.py:60: if key in pdbB: On 2016/12/22 21:35:00, brucedawson wrote: > On 2016/12/22 21:13:20, stanisc wrote: > > if key in pdbA and key in pdbB ? > > The key is guaranteed to be in pdbA because we are iterating through pdbA. What I meant is to replace two "if" statements, this one and the one above, with a combined one with and operator.
https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... File tools/win/pdb_compare_globals.py (right): https://codereview.chromium.org/2592323002/diff/1/tools/win/pdb_compare_globa... tools/win/pdb_compare_globals.py:60: if key in pdbB: I don't think that works because then we don't know what key to check. The 'for' statement means that we go through all of the keys that are in pdbA. That is, the first statement is not an 'if'.
Okay, variables named and redundant lookups avoided. PTAL.
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stanisc@chromium.org Link to the patchset: https://codereview.chromium.org/2592323002/#ps20001 (title: "Code review changes")
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": 20001, "attempt_start_ts": 1482861748514360, "parent_rev": "5f880b10862873800643044a27da4802ed7b71ef", "commit_rev": "814a403c8a0e919ae6e2ca44e70e1d950d2a2fad"}
Message was sent while issue was closed.
Description was changed from ========== Script to compare global variables between two PDBs This script relies on having the recently added ShowGlobals.exe built and in your path. R=stanisc@chromium.org BUG=630755 ========== to ========== Script to compare global variables between two PDBs This script relies on having the recently added ShowGlobals.exe built and in your path. R=stanisc@chromium.org BUG=630755 Review-Url: https://codereview.chromium.org/2592323002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Script to compare global variables between two PDBs This script relies on having the recently added ShowGlobals.exe built and in your path. R=stanisc@chromium.org BUG=630755 Review-Url: https://codereview.chromium.org/2592323002 ========== to ========== Script to compare global variables between two PDBs This script relies on having the recently added ShowGlobals.exe built and in your path. R=stanisc@chromium.org BUG=630755 Committed: https://crrev.com/cc618746e94bb7503d926317abe6d9c37531d35e Cr-Commit-Position: refs/heads/master@{#440764} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/cc618746e94bb7503d926317abe6d9c37531d35e Cr-Commit-Position: refs/heads/master@{#440764} |