Issue 193930043: i#1319: fix improper sorting in callstack table. We connect the sectionClicked signal to our custom…
|
Created:
9 years, 3 months ago by toshi Modified:
9 years, 3 months ago CC:
drmemory-devs_googlegroups.com Visibility:
Public. |
Description-e Commit log for first patchset:
---------------
i#1319: fix improper sorting in callstack table.
We connect the sectionClicked signal to our custom slot that calls our own sorting functions. This guarantees our data is ordered in descending order across all pages.
---------------
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressed reviewer concerns #
Total comments: 6
Patch Set 3 : Addressed review concerns #
Total comments: 4
Patch Set 4 : Committed #
MessagesTotal messages: 18
Contains code style violations: please see https://github.com/DynamoRIO/dynamorio/wiki/Code-Style I think that Branden should have a look as he knows this code best. https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... File drheapstat/visualizer/dhvis_tool.cpp (right): https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:857: /* Static: private sorting functions for use only by std::sort style violation: multi-sentence comment needs punctuation https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:861: sort_by_callstack(const dhvis_callstack_listing_t * v1, const dhvis_callstack_listing_t * v2) style violation: line too long https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:872: /* Only show first 3 (skip 0) frames' func_name style https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:873: * We skip 0 because it is always Dr. Heapstat's replace_malloc() function nit: not always malloc, but yes it will always be replace_* https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:954: bool (*sorting_function)(const dhvis_callstack_listing_t * , const dhvis_callstack_listing_t * ); style violation https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:957: default: case 0: style: cases on own lines
Sign in to reply to this message.
On 2015/01/05 17:04:34, bruening wrote: > Contains code style violations: please see > https://github.com/DynamoRIO/dynamorio/wiki/Code-Style > > I think that Branden should have a look as he knows this code best. > > https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... > File drheapstat/visualizer/dhvis_tool.cpp (right): > > https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... > drheapstat/visualizer/dhvis_tool.cpp:857: /* Static: private sorting functions > for use only by std::sort > style violation: multi-sentence comment needs punctuation > > https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... > drheapstat/visualizer/dhvis_tool.cpp:861: sort_by_callstack(const > dhvis_callstack_listing_t * v1, const dhvis_callstack_listing_t * v2) > style violation: line too long > > https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... > drheapstat/visualizer/dhvis_tool.cpp:872: /* Only show first 3 (skip 0) frames' > func_name > style > > https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... > drheapstat/visualizer/dhvis_tool.cpp:873: * We skip 0 because it is always Dr. > Heapstat's replace_malloc() function > nit: not always malloc, but yes it will always be replace_* > > https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... > drheapstat/visualizer/dhvis_tool.cpp:954: bool (*sorting_function)(const > dhvis_callstack_listing_t * , const dhvis_callstack_listing_t * ); > style violation > > https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... > drheapstat/visualizer/dhvis_tool.cpp:957: default: case 0: > style: cases on own lines Will do, but how do I specify Branden as another reviewer? Also for the switch statement, would simply placing default and case 0 on different lines be sufficient? default: case 0: sorting_function = &sort_by_callstack; break;
Sign in to reply to this message.
On 2015/01/05 17:21:53, DSOTMman wrote: > Will do, but how do I specify Branden as another reviewer? Click on the Edit link to edit the reviewers. > https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... > > drheapstat/visualizer/dhvis_tool.cpp:957: default: case 0: > > style: cases on own lines > > Also for the switch statement, would simply placing default and case 0 on > different lines be sufficient? > default: > case 0: > sorting_function = &sort_by_callstack; break; Yes. Note that it's preferred to reply to the per-line comment in the source file page itself so that the conversation is all contained in that one spot.
Sign in to reply to this message.
Please disregard that last invitation, I neglected to use a comma when specifying the reviewers.
Sign in to reply to this message.
https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... File drheapstat/visualizer/dhvis_tool.cpp (right): https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:857: /* Static: private sorting functions for use only by std::sort On 2015/01/05 17:04:33, bruening wrote: > style violation: multi-sentence comment needs punctuation Done. https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:861: sort_by_callstack(const dhvis_callstack_listing_t * v1, const dhvis_callstack_listing_t * v2) On 2015/01/05 17:04:33, bruening wrote: > style violation: line too long Done. https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:872: /* Only show first 3 (skip 0) frames' func_name On 2015/01/05 17:04:33, bruening wrote: > style Done. https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:873: * We skip 0 because it is always Dr. Heapstat's replace_malloc() function On 2015/01/05 17:04:34, bruening wrote: > nit: not always malloc, but yes it will always be replace_* Should I change this comment? https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:954: bool (*sorting_function)(const dhvis_callstack_listing_t * , const dhvis_callstack_listing_t * ); On 2015/01/05 17:04:34, bruening wrote: > style violation Done. https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:957: default: case 0: On 2015/01/05 17:04:33, bruening wrote: > style: cases on own lines Done.
Sign in to reply to this message.
https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... File drheapstat/visualizer/dhvis_tool.cpp (right): https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:858: * Note that we only will sort by Ascending Order due to limitations in signal/slot Here's the comment that I lost earlier: Can we make this descending instead, so the larger numbers come first? https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:858: * Note that we only will sort by Ascending Order due to limitations in signal/slot Is there a way to flip the sort order as expected when the title buttons are clicked? Maybe by adding state about the sort order and reversing the comparator these functions use?
Sign in to reply to this message.
https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... File drheapstat/visualizer/dhvis_tool.cpp (right): https://codereview.appspot.com/193930043/diff/1/drheapstat/visualizer/dhvis_t... drheapstat/visualizer/dhvis_tool.cpp:858: * Note that we only will sort by Ascending Order due to limitations in signal/slot On 2015/01/07 22:51:24, brandenjclark wrote: > Is there a way to flip the sort order as expected when the title buttons are > clicked? Maybe by adding state about the sort order and reversing the comparator > these functions use? I will try this and probably switch to functors in this case so we don't have a function for each column and for ascending versus descending.
Sign in to reply to this message.
-e Commit log for latest patchset: --------------- i#1319: fix improper sorting in callstack table. We connect the sectionClicked signal to our custom slot that calls our own sorting functions. This guarantees our data is ordered in descending order across all pages. Updated with suggestions. ---------------
Sign in to reply to this message.
On 2015/01/09 03:04:13, DSOTMman wrote: > -e Commit log for latest patchset: > --------------- > i#1319: fix improper sorting in callstack table. > We connect the sectionClicked signal to our custom slot that calls our own > sorting functions. This guarantees our data is ordered in descending order > across all pages. > Updated with suggestions. > --------------- "Updated with suggestions" is good for a per-patchset message on the review site, but not for the commit log which is what will go into the final commit to the repository.
Sign in to reply to this message.
On 2015/01/09 06:00:14, bruening wrote: > On 2015/01/09 03:04:13, DSOTMman wrote: > > -e Commit log for latest patchset: > > --------------- > > i#1319: fix improper sorting in callstack table. > > We connect the sectionClicked signal to our custom slot that calls our own > > sorting functions. This guarantees our data is ordered in descending order > > across all pages. > > Updated with suggestions. > > --------------- > > "Updated with suggestions" is good for a per-patchset message on the review > site, but not for the commit log which is what will go into the final commit to > the repository. Should I reupload to code review with my final commit message?
Sign in to reply to this message.
Besides the minor formatting stuff, LGTM. https://codereview.appspot.com/193930043/diff/20001/drheapstat/visualizer/dhv... File drheapstat/visualizer/dhvis_tool.cpp (right): https://codereview.appspot.com/193930043/diff/20001/drheapstat/visualizer/dhv... drheapstat/visualizer/dhvis_tool.cpp:962: case 1: sorting_function = &sort_by_symbol; break; nit: can we keep these line breaks consistent? https://codereview.appspot.com/193930043/diff/20001/drheapstat/visualizer/dhv... drheapstat/visualizer/dhvis_tool.cpp:972: /* We are garuanteed that if we have to sort by ascending order spelling: guaranteed https://codereview.appspot.com/193930043/diff/20001/drheapstat/visualizer/dhv... drheapstat/visualizer/dhvis_tool.cpp:1674: * sorts the callstack table by appropriate column and reset to first page nit: capitalization (Sorts)
Sign in to reply to this message.
https://codereview.appspot.com/193930043/diff/20001/drheapstat/visualizer/dhv... File drheapstat/visualizer/dhvis_tool.cpp (right): https://codereview.appspot.com/193930043/diff/20001/drheapstat/visualizer/dhv... drheapstat/visualizer/dhvis_tool.cpp:962: case 1: sorting_function = &sort_by_symbol; break; On 2015/01/10 19:35:45, brandenjclark wrote: > nit: can we keep these line breaks consistent? Done. https://codereview.appspot.com/193930043/diff/20001/drheapstat/visualizer/dhv... drheapstat/visualizer/dhvis_tool.cpp:972: /* We are garuanteed that if we have to sort by ascending order On 2015/01/10 19:35:45, brandenjclark wrote: > spelling: guaranteed Done. https://codereview.appspot.com/193930043/diff/20001/drheapstat/visualizer/dhv... drheapstat/visualizer/dhvis_tool.cpp:1674: * sorts the callstack table by appropriate column and reset to first page On 2015/01/10 19:35:45, brandenjclark wrote: > nit: capitalization (Sorts) Done.
Sign in to reply to this message.
-e Commit log for latest patchset: --------------- i#1319: fix improper sorting in callstack table. We connect the sectionClicked signal to our custom slot that calls our own sorting functions. This guarantees our data is ordered in descending order across all pages. ---------------
Sign in to reply to this message.
If comments are addressed then LGTM https://codereview.appspot.com/193930043/diff/40001/drheapstat/visualizer/dhv... File drheapstat/visualizer/dhvis_tool.cpp (right): https://codereview.appspot.com/193930043/diff/40001/drheapstat/visualizer/dhv... drheapstat/visualizer/dhvis_tool.cpp:980: { style violation: { on same line as else https://codereview.appspot.com/193930043/diff/40001/drheapstat/visualizer/dhv... drheapstat/visualizer/dhvis_tool.cpp:1683: * Sorts the callstack table by appropriate column and reset to first page nit: grammar: "sorts" but "reset" nit: "Private slot" here but "Private Slot" on other existing functions: inconsistent
Sign in to reply to this message.
https://codereview.appspot.com/193930043/diff/40001/drheapstat/visualizer/dhv... File drheapstat/visualizer/dhvis_tool.cpp (right): https://codereview.appspot.com/193930043/diff/40001/drheapstat/visualizer/dhv... drheapstat/visualizer/dhvis_tool.cpp:980: { On 2015/01/15 00:36:40, bruening wrote: > style violation: { on same line as else Done. https://codereview.appspot.com/193930043/diff/40001/drheapstat/visualizer/dhv... drheapstat/visualizer/dhvis_tool.cpp:1683: * Sorts the callstack table by appropriate column and reset to first page On 2015/01/15 00:36:40, bruening wrote: > nit: grammar: "sorts" but "reset" > nit: "Private slot" here but "Private Slot" on other existing functions: > inconsistent Done.
Sign in to reply to this message.
-e Committed as https://github.com/DynamoRIO/drmemory/commit/9c1113c2ad9fb3b9162487560b2eef86... Final commit log: --------------- i#1319: fix improper sorting in callstack table. We connect the sectionClicked signal to our custom slot that calls our own sorting functions. This guarantees our data is ordered in descending order across all pages. ---------------
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
