Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844273002:
memory-infra: Add unittest for MemoryDumpManager summary calculations (Closed)
Descriptionmemory-infra: Add unittest for MemoryDumpManager summary calculations
BUG=703184
Review-Url: https://codereview.chromium.org/2844273002
Cr-Commit-Position: refs/heads/master@{#468609}
Committed: https://chromium.googlesource.com/chromium/src/+/09aade854b0637c1da3cb29e93b243393790d534
Patch Set 1 #
Total comments: 10
Patch Set 2 : rebase #Patch Set 3 : address comments #Messages
Total messages: 15 (10 generated)
The CQ bit was checked by hjd@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...
hjd@chromium.org changed reviewers: + fmeawad@chromium.org, primiano@chromium.org
ptal, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with minor comments can you reuse the last CL BUG=? https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:121: // all results are empty. are you intending to keep this "all results are empty" around? If so uppercase A so doesn't look like a leftover https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:306: // trimming off the result argument and calling the global callback. "trimming off" the result confused me a bit before reading the code. IIUC the thing here is that the G callback doesn't take a result, but the P does and you want it. So I'd probably s/by trimming off the result argument.*$/and keeps around the process-local result./ https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:1304: TEST_F(MemoryDumpManagerTest, TestComputingSummary) { s/TestComputingSummary/TestSummaryComputation/ just for consistency with the other test_f above that seem all nouns (% "Test") https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:1328: auto kb = 1024; const uint32_t kB = 1024; kb -> kB will both match our coding style for constants, and also the actual unit abbreviation. This make my OCD so happy. Also I generally tend to avoid auto unless it causes more wrapping or is extremely obvious, as it helps readability. (And integers are the less obvious thing to me in C++, which bite me every other day) https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:1360: can you add a comment explaining why the 99 doesn't show up here. I guess it will be definitely not obvious for anybody who didn't follow all the offline conversations. Another trick I use in this case is to use "malloc/ignored" instead of "foo" in the strings to make it a bit more obvious (although, I realize while suggesting this that it will cause you wrapping over 80 cols. You have my compassion)
Thanks! Updated :) https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:121: // all results are empty. On 2017/04/28 15:04:33, Primiano Tucci wrote: > are you intending to keep this "all results are empty" around? > If so uppercase A so doesn't look like a leftover oops, thanks! Done. https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:306: // trimming off the result argument and calling the global callback. On 2017/04/28 15:04:33, Primiano Tucci wrote: > "trimming off" the result confused me a bit before reading the code. > IIUC the thing here is that the G callback doesn't take a result, but the P does > and you want it. > So I'd probably s/by trimming off the result argument.*$/and keeps around the > process-local result./ Done. https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:1304: TEST_F(MemoryDumpManagerTest, TestComputingSummary) { On 2017/04/28 15:04:33, Primiano Tucci wrote: > s/TestComputingSummary/TestSummaryComputation/ > > just for consistency with the other test_f above that seem all nouns (% "Test") Done. https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:1328: auto kb = 1024; On 2017/04/28 15:04:33, Primiano Tucci wrote: > const uint32_t kB = 1024; > kb -> kB will both match our coding style for constants, and also the actual > unit abbreviation. This make my OCD so happy. > > Also I generally tend to avoid auto unless it causes more wrapping or is > extremely obvious, as it helps readability. (And integers are the less obvious > thing to me in C++, which bite me every other day) > Done. https://codereview.chromium.org/2844273002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:1360: On 2017/04/28 15:04:33, Primiano Tucci wrote: > can you add a comment explaining why the 99 doesn't show up here. > I guess it will be definitely not obvious for anybody who didn't follow all the > offline conversations. > Another trick I use in this case is to use "malloc/ignored" instead of "foo" in > the strings to make it a bit more obvious (although, I realize while suggesting > this that it will cause you wrapping over 80 cols. You have my compassion) Yeah XD I ended up wrapping some of the others anyway though so I've gone ahead and updated it now.
Description was changed from ========== memory-infra: Add unittest for MemoryDumpManager summary calculations ========== to ========== memory-infra: Add unittest for MemoryDumpManager summary calculations BUG=703184 ==========
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2844273002/#ps40001 (title: "address comments")
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": 40001, "attempt_start_ts": 1493728803394200,
"parent_rev": "72a220118ee319f86c84d6062ceae9200cb9e710", "commit_rev":
"09aade854b0637c1da3cb29e93b243393790d534"}
Message was sent while issue was closed.
Description was changed from ========== memory-infra: Add unittest for MemoryDumpManager summary calculations BUG=703184 ========== to ========== memory-infra: Add unittest for MemoryDumpManager summary calculations BUG=703184 Review-Url: https://codereview.chromium.org/2844273002 Cr-Commit-Position: refs/heads/master@{#468609} Committed: https://chromium.googlesource.com/chromium/src/+/09aade854b0637c1da3cb29e93b2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/09aade854b0637c1da3cb29e93b2... |
