repl: tab auto complete big arrays by BridgeAR · Pull Request #22408 · nodejs/node

BridgeAR · 2018-08-20T01:31:55Z

Due to a new API it's possible to skip the indices. That allows to
use auto completion with big (typed) arrays.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-08-20T01:31:56Z

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/599/pipeline

BridgeAR · 2018-08-20T01:32:17Z

@nodejs/repl PTAL

benjamingr

Nice work, not sure about semverness.

LGTM pending green CI. Left some nits I'd appreciate if you addressed but I'm still green with them.

Mostly - more tests would be fantastic.

benjamingr · 2018-08-20T14:32:48Z

lib/internal/util.js

Would it make sense to copy the whole enum as it is in V8 ( https://github.com/v8/v8/blob/master/src/property-details.h#L36-L45 ) for consistency and keeping a reference to the V8 source here?

So far, we usually go all the way here and provide the real V8 constants as exports from the binding, e.g.

node/src/pipe_wrap.cc

Lines 105 to 114 in 9bcb744

// Define constants

Local<Object> constants = Object::New(env->isolate());

NODE_DEFINE_CONSTANT(constants, SOCKET);

NODE_DEFINE_CONSTANT(constants, SERVER);

NODE_DEFINE_CONSTANT(constants, IPC);

NODE_DEFINE_CONSTANT(constants, UV_READABLE);

NODE_DEFINE_CONSTANT(constants, UV_WRITABLE);

target->Set(context,

FIXED_ONE_BYTE_STRING(env->isolate(), "constants"),

constants).FromJust();

I’d encourage you to keep up with that pattern to prevent unexpected breakage from changes to the V8 definition, which is the source-of-truth for these values, even if it seems like a bit of extra work at this point (or leave to TODO comment for me, I’m happy to do the work myself if you prefer)

Thanks a lot for the comments. I definitely appreciate it as I try to teach myself more and more about C++ and the V8 side. So please always mention these things.

benjamingr · 2018-08-20T14:34:04Z

lib/internal/util/comparisons.js

Nit: while I use this style of destructuring in my own code - a common criticism I've found is that it's more confusing and in this case it might make sense to import the whole filter similarly to how V8 does PropertyFilter:: ALL_PROPERTIES although scoping rules would have allowed it to use the unqualified name allow.

benjamingr · 2018-08-20T14:35:51Z

lib/internal/util/comparisons.js

Unrelated optimization idea: would it make sense to return only the length from V8 rather than the properties themselves in the case of keys2?

I already spoke with @nodejs/v8 to implement a API that does exactly that. Until that exists, I would rather keep it as is.

benjamingr · 2018-08-20T14:38:49Z

src/node_util.cc

Wouldn't it make sense for this to be a CHECK since we should never end up here without said args?

(Not critical for this PR)

benjamingr · 2018-08-20T14:40:32Z

test/parallel/test-repl-tab-complete.js

It would be great to have tests that specify the behaviour for long arrays better.

This test is specifically for long arrays. Since this value shows up, we can be certain that it works. I would not know what to improve. Do you have a suggestion?

addaleax · 2018-08-20T14:47:42Z

src/node_util.cc

Since you already know at this point that args[1] is an Uint32 here, I’d suggest using args[1].As<Uint32>()->Value() (which is a static cast that fully compiles away, whereas Uint32Value() performs the equivalent of value|0 in JS, i.e. performs a real conversion that can potentially run userland code for non-integer input)

addaleax · 2018-08-20T14:51:45Z

lib/internal/util.js

So far, we usually go all the way here and provide the real V8 constants as exports from the binding, e.g.

node/src/pipe_wrap.cc

Lines 105 to 114 in 9bcb744

// Define constants

Local<Object> constants = Object::New(env->isolate());

NODE_DEFINE_CONSTANT(constants, SOCKET);

NODE_DEFINE_CONSTANT(constants, SERVER);

NODE_DEFINE_CONSTANT(constants, IPC);

NODE_DEFINE_CONSTANT(constants, UV_READABLE);

NODE_DEFINE_CONSTANT(constants, UV_WRITABLE);

target->Set(context,

FIXED_ONE_BYTE_STRING(env->isolate(), "constants"),

constants).FromJust();

I’d encourage you to keep up with that pattern to prevent unexpected breakage from changes to the V8 definition, which is the source-of-truth for these values, even if it seems like a bit of extra work at this point (or leave to TODO comment for me, I’m happy to do the work myself if you prefer)

jdalton · 2018-08-20T15:02:49Z

lib/internal/util/comparisons.js

The reason for needing getOwnNonIndexProperties should be brought up to the TC39 (if it hasn't already). It'd be good to let them know that Node is having to use engine level APIs to work around a JS lang limitation (no way to return a value with methods that return arrays of keys).

I already spoke to @littledan and I am writing the proposal at the moment. It's almost done.

Due to a new API it's possible to skip the indices. That allows to use auto completion with big (typed) arrays. Fixes: nodejs#21446

BridgeAR · 2018-08-20T15:51:37Z

Comments addressed. PTAL

CI https://ci.nodejs.org/job/node-test-pull-request/16612/

addaleax

Just reaffirming my LGTM here :)

Due to a new API it's possible to skip the indices. That allows to use auto completion with big (typed) arrays. PR-URL: nodejs#22408 Fixes: nodejs#21446 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

BridgeAR · 2018-08-22T22:12:03Z

Landed in 68b07de 🎉

Due to a new API it's possible to skip the indices. That allows to use auto completion with big (typed) arrays. PR-URL: #22408 Fixes: #21446 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 20, 2018

BridgeAR requested review from addaleax and ryzokuken August 20, 2018 01:32

benjamingr approved these changes Aug 20, 2018

View reviewed changes

addaleax approved these changes Aug 20, 2018

View reviewed changes

jdalton reviewed Aug 20, 2018

View reviewed changes

BridgeAR added 2 commits August 20, 2018 17:47

repl: tab auto complete big arrays

3f26e1f

Due to a new API it's possible to skip the indices. That allows to use auto completion with big (typed) arrays. Fixes: nodejs#21446

fixup: address comments

7be3845

BridgeAR force-pushed the auto-complete-big-arrays branch from 5c0904f to 7be3845 Compare August 20, 2018 15:48

fixup: address c++ linter

65993be

addaleax added the repl Issues and PRs related to the REPL subsystem. label Aug 20, 2018

addaleax approved these changes Aug 20, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 20, 2018

BridgeAR closed this Aug 22, 2018

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

BridgeAR deleted the auto-complete-big-arrays branch January 20, 2020 11:37

	// Define constants
	Local<Object> constants = Object::New(env->isolate());
	NODE_DEFINE_CONSTANT(constants, SOCKET);
	NODE_DEFINE_CONSTANT(constants, SERVER);
	NODE_DEFINE_CONSTANT(constants, IPC);
	NODE_DEFINE_CONSTANT(constants, UV_READABLE);
	NODE_DEFINE_CONSTANT(constants, UV_WRITABLE);
	target->Set(context,
	FIXED_ONE_BYTE_STRING(env->isolate(), "constants"),
	constants).FromJust();

Uh oh!

Conversation

BridgeAR commented Aug 20, 2018

Checklist

Uh oh!

nodejs-github-bot commented Aug 20, 2018

Uh oh!

BridgeAR commented Aug 20, 2018

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 20, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants