Skip to content

Add iterator for ASTNodeArray #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maxbrunsfeld merged 5 commits into from
Dec 28, 2017
Merged

Add iterator for ASTNodeArray #8

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce97d35
Add iterator for ASTNodeArray
Xapphire13 Dec 28, 2017
3febdf8
Adding ASTNodeArray to exports
Xapphire13 Dec 28, 2017
8577018
add iterator to prototype
Xapphire13 Dec 28, 2017
e287eb3
Adding IsNamed to ASTNodeArray
Xapphire13 Dec 28, 2017
27a2ec0
Un-document AstNodeArray.isNamed
Xapphire13 Dec 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const {Document, ASTNode, pointTransferArray} = require('./build/Release/tree_sitter_runtime_binding')
const {Document, ASTNode, ASTNodeArray, pointTransferArray} = require('./build/Release/tree_sitter_runtime_binding')

class StringInput {
constructor (string, bufferSize) {
Expand All @@ -24,6 +24,21 @@ Document.prototype.setInputString = function (string, bufferSize) {
return this
}

ASTNodeArray.prototype[Symbol.iterator] = function* () {
let node = this[0];

const getNext = this.isNamed ?
(node) => node.nextNamedSibling :
(node) => node.nextSibling;

if (node) {
yield node;
while ((node = getNext(node))) {
yield node;
}
Copy link
Contributor

@maxbrunsfeld maxbrunsfeld Dec 28, 2017

Choose a reason for hiding this comment

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

Even though it will mean more duplication, I'd prefer to have one conditional check at the beginning of the loop, rather than a closure with a conditional on each iteration. Something like:

if (this.isNamed) {
  while ((node = node.nextNamedSibling)) {
    yield node
  }
} else {
  while ((node = node.nextSibling)) {
    yield node
  }
}

Copy link
Contributor Author

@Xapphire13 Xapphire13 Dec 28, 2017

Choose a reason for hiding this comment

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

I can do this, but I wanted to mention that the way it's currently implemented doesn't use a closure and the this.isNamed is only evaluated once. The only difference this change would make would be removing the stack frame of the lambda on lines 31/32.

Copy link
Contributor

@maxbrunsfeld maxbrunsfeld Dec 28, 2017

Choose a reason for hiding this comment

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

Ah, thanks for pointing that out; I misread your code.

}
}

const {startPosition, endPosition} = ASTNode.prototype

Object.defineProperty(ASTNode.prototype, 'startPosition', {
Expand Down
5 changes: 3 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 12 additions & 1 deletion src/ast_node_array.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ using namespace v8;

Nan::Persistent<Function> ASTNodeArray::constructor;

void ASTNodeArray::Init() {
void ASTNodeArray::Init(v8::Local<v8::Object> exports) {
Local<FunctionTemplate> tpl = Nan::New<FunctionTemplate>(New);
tpl->SetClassName(Nan::New("ASTNodeArray").ToLocalChecked());
tpl->InstanceTemplate()->SetInternalFieldCount(1);
Expand All @@ -28,6 +28,11 @@ void ASTNodeArray::Init() {
Nan::New("length").ToLocalChecked(),
Length, NULL);

Nan::SetAccessor(
tpl->InstanceTemplate(),
Nan::New("isNamed").ToLocalChecked(),
IsNamed);

const char * array_methods[] = {
"every",
"filter",
Expand All @@ -51,6 +56,7 @@ void ASTNodeArray::Init() {
prototype->Set(method_name, array->Get(method_name));
}

exports->Set(Nan::New("ASTNodeArray").ToLocalChecked(), constructor_local);
constructor.Reset(Nan::Persistent<Function>(constructor_local));
}

Expand Down Expand Up @@ -91,4 +97,9 @@ void ASTNodeArray::Length(Local<String> property, const Nan::PropertyCallbackInf
info.GetReturnValue().Set(Nan::New<Number>(length));
}

void ASTNodeArray::IsNamed(Local<String> property, const Nan::PropertyCallbackInfo<Value> &info) {
ASTNodeArray *array = ObjectWrap::Unwrap<ASTNodeArray>(info.This());
info.GetReturnValue().Set(array->is_named_);
}

} // namespace node_tree_sitter
3 changes: 2 additions & 1 deletion src/ast_node_array.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace node_tree_sitter {

class ASTNodeArray : public Nan::ObjectWrap {
public:
static void Init();
static void Init(v8::Local<v8::Object> exports);
static v8::Local<v8::Value> NewInstance(TSNode, TSDocument *, size_t, bool);

private:
Expand All @@ -19,6 +19,7 @@ class ASTNodeArray : public Nan::ObjectWrap {
static void New(const Nan::FunctionCallbackInfo<v8::Value> &);
static void Length(v8::Local<v8::String>, const Nan::PropertyCallbackInfo<v8::Value> &);
static void GetIndex(uint32_t, const Nan::PropertyCallbackInfo<v8::Value> &);
static void IsNamed(v8::Local<v8::String>, const Nan::PropertyCallbackInfo<v8::Value> &);

TSNode parent_node_;
TSDocument *document_;
Expand Down
2 changes: 1 addition & 1 deletion src/binding.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ using namespace v8;
void InitAll(Local<Object> exports) {
InitConversions();
ASTNode::Init(exports);
ASTNodeArray::Init();
ASTNodeArray::Init(exports);
InputReader::Init();
Document::Init(exports);
InitConversions();
Expand Down
6 changes: 5 additions & 1 deletion tree-sitter.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ declare module "tree-sitter" {

type ArrayCallback<TItem, TArray, TReturn> = (node: TItem, index?: number, array?: TArray) => TReturn

export interface AstNodeArray extends ArrayLike<AstNode> {
export interface AstNodeArray extends ArrayLike<AstNode>, Iterable<AstNode> {
// Getters
isNamed: boolean;
Copy link
Contributor

@maxbrunsfeld maxbrunsfeld Dec 28, 2017

Choose a reason for hiding this comment

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

I think we shouldn't document this property; let's just treat it as an implementation detail of node arrays.

Copy link
Contributor Author

@Xapphire13 Xapphire13 Dec 28, 2017

Choose a reason for hiding this comment

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

I thought that too, I was just concerned about someone seeing it later down the line and thinking "Hey, this isn't documented, must be a bug". If only JS had true private methods =]


// Methods
map<T>(callback: ArrayCallback<AstNode, AstNodeArray, void>, thisArg?: any): T[];
every(callback: ArrayCallback<AstNode, AstNodeArray, void>, thisArg?: any): void;
filter(callback: ArrayCallback<AstNode, AstNodeArray, boolean>, thisArg?: any): AstNode[];
Expand Down