src: add recursive option to `findrefs` command by Drieger · Pull Request #244 · nodejs/llnode

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

      " * -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

Uh oh!

Conversation

Drieger commented Oct 23, 2018

Uh oh!

mmarchini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmarchini Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Drieger commented Oct 29, 2018

Uh oh!

Uh oh!

mmarchini Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

Drieger commented Jan 21, 2019

Uh oh!

Drieger commented Jan 30, 2019

Uh oh!

codecov-io commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mmarchini left a comment

Choose a reason for hiding this comment

Uh oh!

mmarchini commented Feb 26, 2019

Uh oh!

mmarchini commented Feb 26, 2019

Uh oh!

mmarchini commented Mar 5, 2019

Uh oh!

fanatid Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Feb 11, 2019 •

edited

Loading