src: add iterator for Object #930

RaisinTen · 2021-03-12T16:57:31Z

Refs: #830
Signed-off-by: Darshan Sen darshan.sen@postman.com

RaisinTen · 2021-03-12T16:57:58Z

I made this very naive implementation of something that looks like an iterator for Napi::Object. Could we please discuss about how to implement this here? If I'm not wrong, in the C++ STL, RB trees are used to keep track of the sequence of iteration but I'm not sure what to use here other than what I have done below.

devsnek · 2021-03-12T16:59:19Z

Can this work with c++ exceptions disabled?

RaisinTen · 2021-03-12T17:05:12Z

Can this work with c++ exceptions disabled?

Does all of Napi::* work when c++ exceptions are disabled?

devsnek · 2021-03-12T17:11:01Z

yes, using NAPI_DISABLE_CPP_EXCEPTIONS

RaisinTen · 2021-03-13T13:57:06Z

@devnek No error checking is implemented yet. I will keep in mind that it works without c++ exceptions.

gabrielschulhof · 2021-03-19T15:21:33Z

@RaisinTen could you please add a test which preferably also includes a case where an exception is thrown during iteration?

RaisinTen · 2021-03-20T16:55:28Z

@gabrielschulhof Do you mean a test case where the object _object is pointing to gets freed while iterating? Also, is there a way of getting a pointer to the underlying object keys array of the object that is being iterated? The current implementation keeps a copy of the object keys array when std::begin() is called on the object.

aminya · 2021-04-09T22:25:09Z

@gabrielschulhof Do you mean a test case where the object _object is pointing to gets freed while iterating? Also, is there a way of getting a pointer to the underlying object keys array of the object that is being iterated? The current implementation keeps a copy of the object keys array when std::begin() is called on the object.

I think a for ranged loop over a Napi::Array and maybe calling one of the STL functions like std::transform would be suitable test cases.

RaisinTen · 2021-04-10T15:35:01Z

Since most of the tests check that the behaviour from the C++ side is coherent with the JS side, I wrote a test to check that the computed sum of a Napi::Array using the iterator is the same from both the JS side and the C++ side. Since I used the for ranged loop already, I didn't use std::accumulate on this.

Still not very sure, what I should do regarding exceptions though. Should we throw std::out_of_range when anything looks fishy inside the iterator?

napi-inl.h

RaisinTen · 2021-04-10T15:40:14Z

test/object/object.cc

+  int64_t sum = 0;
+
+  for (const auto& e : array) {
+    sum += e.second.As<Number>().Int64Value();


Perhaps, I should change this test to iterate over an object because I'm planning on adding a separate iterator for Napi::Array, where the Napi::Array::operator*() would return the Value& at the currently iterated index.

Having a special iterator for arrays would be nice.

RaisinTen · 2021-04-12T16:10:19Z

cc @gabrielschulhof, @mhdawson Could this please get another review?

test/object/object.js

gabrielschulhof · 2021-07-09T15:18:30Z

napi.h

@@ -598,6 +598,14 @@ namespace Napi {
      uint32_t index /// Property / element index
    );

+    /// Gets or sets an indexed property or array element.
+    PropertyLValue<Value> operator[](Value index  /// Property / element index


This whitespacing is strange. Please close the bracket immediately after index! Please use // for comments!

@gabrielschulhof I agree, it does look strange. It was done by the linter though. I used /// for the comments to make it consistent with the rest of the comments nearby. Perhaps we can change all comments to use // in a separate PR?

gabrielschulhof · 2021-07-09T15:21:13Z

napi.h

+
+    inline iterator end();
+#endif  // NAPI_CPP_EXCEPTIONS
+


Why do we need these forward declarations? Why not place the const_iterator and iterator class definitions in directly here.

@gabrielschulhof The iterator classes require the definition of the Napi::Array class to be completed but it is not completed here yet. That's why I decided to move the actual class definitions after the definition of Napi::Array.

KevinEady

I'm not a big fan of the changes made in order to see if the binding has C++ exceptions enabled. Is it possible to change the logic?

test/common/index.js

test/object/object.js

RaisinTen · 2021-08-28T14:19:19Z

Been a while since the last review. Can this please get another review as I have addressed all the comments and rebased?
cc @gabrielschulhof @KevinEady @legendecas @mhdawson

mhdawson · 2021-09-08T17:27:30Z

@RaisinTen I looked through it and the main thing I think it needs is additions to the documentation. @gabrielschulhof @KevinEady @legendecas anything else you want to point out?

RaisinTen · 2021-09-11T07:42:11Z

@mhdawson added docs, PTAL :)

doc/object.md

mhdawson

LGTM with a suggestion

Refs: nodejs#830 Signed-off-by: Darshan Sen <darshan.sen@postman.com>

devnek · 2021-09-26T15:29:34Z

@devnek No error checking is implemented yet. I will keep in mind that it works without c++ exceptions.

Please don't tag @devnek

mhdawson · 2021-09-28T14:46:15Z

@NickNaso I think you wanted to do a pass through before this landed.

NickNaso · 2021-09-28T14:59:07Z

@NickNaso I think you wanted to do a pass through before this landed.

Yes, I'm taking a look right now.

NickNaso

@RaisinTen just some notes about documentation.

NickNaso · 2021-09-29T08:19:47Z

doc/object.md

+### Constant Iterator
+
+In constant iterators, the iterated values are immutable.
+


Here you should document the interface exposed by Napi::Object::const_iterator.

NickNaso · 2021-09-29T08:20:33Z

doc/object.md

+
+In constant iterators, the iterated values are immutable.
+
+```cpp


Here you should add title Example

NickNaso · 2021-09-29T08:21:52Z

doc/object.md

+### Non Constant Iterator
+
+In non constant iterators, the iterated values are mutable.
+


Here you should document the interface exposed by Napi::Object::terator

NickNaso · 2021-09-29T08:22:35Z

doc/object.md

+
+In non constant iterators, the iterated values are mutable.
+
+```cpp


Here you should add title Example.

RaisinTen · 2021-10-03T15:52:38Z

@NickNaso thank you very much for the review comments. Before I address the comments, I would like to know whether I should add the changes in a separate PR because the changes in this PR seems to be landed already in bd8f6e6.
cc @mhdawson for some clarification.

mhdawson · 2021-10-04T18:42:31Z

@RaisinTen thanks for noticing that. I think I landed it accidentally as part of landing another PR. I think making those changes in a new PR is the best way to go.

mhdawson · 2021-10-08T15:08:03Z

Closing since landed and we can fix the suggestions in a follow up.

RaisinTen marked this pull request as draft March 12, 2021 16:58

RaisinTen mentioned this pull request Mar 13, 2021

Define iterator for Napi::Array for using with ranged for-loop or STL algorithms #830

Closed

RaisinTen marked this pull request as ready for review March 13, 2021 15:25

RaisinTen force-pushed the src/add-iterator-for-Object branch 2 times, most recently from 490a387 to c046a43 Compare March 13, 2021 15:31

RaisinTen mentioned this pull request Apr 7, 2021

No NAPI access to Javascript Map objects nodejs/help#3303

Closed

RaisinTen force-pushed the src/add-iterator-for-Object branch from c046a43 to 02186ee Compare April 8, 2021 15:37

RaisinTen commented Apr 10, 2021

View reviewed changes

napi-inl.h Outdated Show resolved Hide resolved

RaisinTen commented Apr 10, 2021

View reviewed changes

legendecas reviewed Apr 14, 2021

View reviewed changes

test/object/object.js Show resolved Hide resolved

RaisinTen force-pushed the src/add-iterator-for-Object branch from 8b0da48 to ca0a067 Compare June 16, 2021 15:44

trxcllnt mentioned this pull request Jun 28, 2021

Add DataFrame.fromArrow rapidsai/node#250

Merged

gabrielschulhof reviewed Jul 9, 2021

View reviewed changes

RaisinTen force-pushed the src/add-iterator-for-Object branch from dc30134 to 0a40317 Compare July 11, 2021 04:52

KevinEady reviewed Jul 11, 2021

View reviewed changes

test/common/index.js Outdated Show resolved Hide resolved

test/object/object.js Outdated Show resolved Hide resolved

test/object/object.js Outdated Show resolved Hide resolved

RaisinTen force-pushed the src/add-iterator-for-Object branch from 0a40317 to 176657b Compare July 18, 2021 06:16

RaisinTen requested review from gabrielschulhof, legendecas and KevinEady July 18, 2021 06:20

RaisinTen force-pushed the src/add-iterator-for-Object branch from 176657b to 98c26e0 Compare August 28, 2021 14:16

RaisinTen force-pushed the src/add-iterator-for-Object branch from 98c26e0 to ebdb98a Compare August 28, 2021 14:28

RaisinTen force-pushed the src/add-iterator-for-Object branch 3 times, most recently from 2775157 to 3835c91 Compare September 11, 2021 07:40

mhdawson reviewed Sep 13, 2021

View reviewed changes

doc/object.md Outdated Show resolved Hide resolved

mhdawson approved these changes Sep 13, 2021

View reviewed changes

mhdawson mentioned this pull request Sep 13, 2021

Stale issues in node-addon-api to discuss nodejs/abi-stable-node#415

Open

src: add iterator for Object

ed6c419

Refs: nodejs#830 Signed-off-by: Darshan Sen <darshan.sen@postman.com>

RaisinTen force-pushed the src/add-iterator-for-Object branch from 3835c91 to ed6c419 Compare September 14, 2021 14:09

NickNaso reviewed Sep 29, 2021

View reviewed changes

mhdawson closed this Oct 8, 2021

RaisinTen deleted the src/add-iterator-for-Object branch October 8, 2021 16:05

RaisinTen mentioned this pull request Oct 10, 2021

doc: document object iterators #1090

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add iterator for Object #930

src: add iterator for Object #930

RaisinTen commented Mar 12, 2021 •

edited

Loading

RaisinTen commented Mar 12, 2021

devsnek commented Mar 12, 2021

RaisinTen commented Mar 12, 2021

devsnek commented Mar 12, 2021

RaisinTen commented Mar 13, 2021

gabrielschulhof commented Mar 19, 2021

RaisinTen commented Mar 20, 2021

aminya commented Apr 9, 2021 •

edited

Loading

RaisinTen commented Apr 10, 2021

RaisinTen Apr 10, 2021 •

edited

Loading

aminya Apr 14, 2021

RaisinTen commented Apr 12, 2021

gabrielschulhof Jul 9, 2021

RaisinTen Jul 11, 2021

gabrielschulhof Jul 9, 2021

RaisinTen Jul 11, 2021

KevinEady left a comment

RaisinTen commented Aug 28, 2021

mhdawson commented Sep 8, 2021

RaisinTen commented Sep 11, 2021

mhdawson left a comment

devnek commented Sep 26, 2021

mhdawson commented Sep 28, 2021

NickNaso commented Sep 28, 2021

NickNaso left a comment

NickNaso Sep 29, 2021

NickNaso Sep 29, 2021

NickNaso Sep 29, 2021

NickNaso Sep 29, 2021

RaisinTen commented Oct 3, 2021

mhdawson commented Oct 4, 2021

mhdawson commented Oct 8, 2021

		### Constant Iterator

		In constant iterators, the iterated values are immutable.


		In constant iterators, the iterated values are immutable.

		```cpp

		### Non Constant Iterator

		In non constant iterators, the iterated values are mutable.

src: add iterator for Object #930

src: add iterator for Object #930

Conversation

RaisinTen commented Mar 12, 2021 • edited Loading

RaisinTen commented Mar 12, 2021

devsnek commented Mar 12, 2021

RaisinTen commented Mar 12, 2021

devsnek commented Mar 12, 2021

RaisinTen commented Mar 13, 2021

gabrielschulhof commented Mar 19, 2021

RaisinTen commented Mar 20, 2021

aminya commented Apr 9, 2021 • edited Loading

RaisinTen commented Apr 10, 2021

RaisinTen Apr 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaisinTen commented Apr 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinEady left a comment

Choose a reason for hiding this comment

RaisinTen commented Aug 28, 2021

mhdawson commented Sep 8, 2021

RaisinTen commented Sep 11, 2021

mhdawson left a comment

Choose a reason for hiding this comment

devnek commented Sep 26, 2021

mhdawson commented Sep 28, 2021

NickNaso commented Sep 28, 2021

NickNaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaisinTen commented Oct 3, 2021

mhdawson commented Oct 4, 2021

mhdawson commented Oct 8, 2021

RaisinTen commented Mar 12, 2021 •

edited

Loading

aminya commented Apr 9, 2021 •

edited

Loading

RaisinTen Apr 10, 2021 •

edited

Loading