src: add recursive option to `findrefs` command #244

Drieger · 2018-10-23T18:48:44Z

Introduce a new command flag to findrefs, command --recursive
allow user to transverse the whole references tree for the given
object.

Refs: #115

mmarchini

Nice! :D

Left a few comments. Also, need to run make format here.

src/llscan.cc

mmarchini · 2018-10-23T19:42:04Z

src/llscan.cc

+        ss << rang::fg::cyan << "0x%" PRIx64  << rang::fg::reset << ": "
+           << rang::fg::magenta << "Context" << rang::style::bold << rang::fg::yellow
+           << ".%s" << rang::fg::reset << rang::style::reset << "=" << rang::fg::cyan
+           << "0x%" PRIx64 << rang::fg::reset << "\n";


Can't we call Context::Inspect instead of reformatting it here?

src/llnode.cc

Drieger · 2018-10-29T10:55:14Z

@mmarchini I already addressed your comments.

src/llscan.cc

mmarchini · 2019-01-18T15:38:50Z

src/llnode.cc

@@ -491,6 +522,7 @@ bool PluginInitialize(SBDebugger d) {
      " * -n, --name  name     - all properties with the specified name\n"
      " * -s, --string string  - all properties that refer to the specified "
      "JavaScript string value\n"
+      " * -r, --recursive      - walk through references tree recursively\n"


-r is incompatible with -s and -n, right? If so this should be documented, otherwise, we should have tests for -r -s <string> and -r -n <property>.

Drieger · 2019-01-21T17:45:14Z

@mmarchini I moved duplicated code to a method and added tests for all findrefs options.

Drieger · 2019-01-30T15:50:49Z

The error seems to be related to #267 and was not introduced by this PR, it is being captured by the test in some situations.

codecov-io · 2019-02-11T13:31:06Z

Codecov Report

Merging #244 into master will increase coverage by 0.81%.
The diff coverage is 64.03%.

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage   79.96%   80.77%   +0.81%     
==========================================
  Files          34       34              
  Lines        4292     4369      +77     
==========================================
+ Hits         3432     3529      +97     
+ Misses        860      840      -20

Impacted Files	Coverage Δ
src/settings.cc	`78.94% <0%> (-21.06%)`	⬇️
test/plugin/scan-test.js	`98.91% <100%> (+0.36%)`	⬆️
src/llscan.h	`68.18% <50%> (+15.87%)`	⬆️
src/llnode.h	`50% <50%> (ø)`	⬆️
src/llnode.cc	`73.12% <6.66%> (-4.19%)`	⬇️
src/llscan.cc	`60.73% <68.75%> (+5.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea06ef5...6b4e80a. Read the comment docs.

Introduce a new command flag to `findrefs`, command --recursive allow user to transverse the whole references tree for the given object. Refs: nodejs#115

…ndrefs

mmarchini

LGTM. There's still some cleanup to be done in the future, but nothing blocker. Thanks @Drieger!

mmarchini · 2019-02-26T13:44:06Z

This is the result for a recursive findrefs on Zlib:

This feature will be really helpful 😄

mmarchini · 2019-02-26T13:44:54Z

I'll keep this open for a few days in case someone else wants to review it, otherwise I plan to land this by the weekend.

Introduce a new command flag to `findrefs`, command --recursive allow user to transverse the whole references tree for the given object. Refs: #115 PR-URL: #244 Reviewed-By: Matheus Marchini <mat@mmarchini.me>

PR-URL: #244 Refs: #115 Reviewed-By: Matheus Marchini <mat@mmarchini.me>

mmarchini · 2019-03-05T12:03:26Z

Landed in 85f067a...c29d5af, thank you!

fanatid · 2019-09-24T13:02:15Z

src/llnode.h

+                 lldb::SBCommandReturnObject& result) override;
+
+ private:
+  v8::LLV8* llv8_;


llv8_ unused, PR with fix #297

mmarchini reviewed Oct 23, 2018

View reviewed changes

mmarchini reviewed Jan 18, 2019

View reviewed changes

Drieger force-pushed the findrefstree_command branch 16 times, most recently from dbda298 to 240cbbb Compare January 30, 2019 15:40

mmarchini force-pushed the findrefstree_command branch from 240cbbb to 65d202a Compare February 11, 2019 13:28

Drieger and others added 4 commits February 25, 2019 16:25

src: add recursive option to findrefs command

4064768

Introduce a new command flag to `findrefs`, command --recursive allow user to transverse the whole references tree for the given object. Refs: nodejs#115

fixup! make sure references by value are initialized for recursive fi…

e97ba6e

…ndrefs

fixup! fix indent issue + make the output more compact

05c9395

test: disable flaky test for findrefs -s

6b4e80a

mmarchini force-pushed the findrefstree_command branch from 51cfe57 to 6b4e80a Compare February 25, 2019 19:26

mmarchini approved these changes Feb 26, 2019

View reviewed changes

mmarchini added the author ready PRs that have at least one approvals, no pending requests for changes, and a CI started. label Feb 26, 2019

mmarchini added a commit that referenced this pull request Mar 5, 2019

test: disable flaky test for findrefs -s

c29d5af

PR-URL: #244 Refs: #115 Reviewed-By: Matheus Marchini <mat@mmarchini.me>

mmarchini closed this Mar 5, 2019

mmarchini mentioned this pull request Mar 5, 2019

v8 findrefstree command #115

Closed

fanatid reviewed Sep 24, 2019

View reviewed changes

src/llnode.h

lldb::SBCommandReturnObject& result) override;

private:

v8::LLV8* llv8_;

Copy link

Contributor

fanatid Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llv8_ unused, PR with fix #297

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add recursive option to `findrefs` command #244

src: add recursive option to `findrefs` command #244

Drieger commented Oct 23, 2018

mmarchini left a comment

mmarchini Oct 23, 2018

Drieger commented Oct 29, 2018

mmarchini Jan 18, 2019

Drieger commented Jan 21, 2019

Drieger commented Jan 30, 2019

codecov-io commented Feb 11, 2019 •

edited

Loading

mmarchini left a comment

mmarchini commented Feb 26, 2019

mmarchini commented Feb 26, 2019

mmarchini commented Mar 5, 2019

fanatid Sep 24, 2019

src: add recursive option to findrefs command #244

src: add recursive option to findrefs command #244

Conversation

Drieger commented Oct 23, 2018

mmarchini left a comment

Choose a reason for hiding this comment

mmarchini Oct 23, 2018

Choose a reason for hiding this comment

Drieger commented Oct 29, 2018

mmarchini Jan 18, 2019

Choose a reason for hiding this comment

Drieger commented Jan 21, 2019

Drieger commented Jan 30, 2019

codecov-io commented Feb 11, 2019 • edited Loading

Codecov Report

mmarchini left a comment

Choose a reason for hiding this comment

mmarchini commented Feb 26, 2019

mmarchini commented Feb 26, 2019

mmarchini commented Mar 5, 2019

fanatid Sep 24, 2019

Choose a reason for hiding this comment

src: add recursive option to `findrefs` command #244

src: add recursive option to `findrefs` command #244

codecov-io commented Feb 11, 2019 •

edited

Loading