src,build: improve the native module subsystem #36

aqrln · 2017-01-30T23:48:05Z

Split jsrs-impl.cc into separate modules and make stylistic fixes.
Rename the native addon to jstp since there already is a function that is not a part of JSRS.
Fix binding.gyp: make cflags not ignored on macOS (as it appeared they used to be) and do not use -O3 in Debug configuration.
Make v8::String instantiations and throwing V8 exceptions macros to avoid boilerplate code.

* Split `jsrs-impl.cc` into separate modules. * Make some refactoring. * Rename the native addon to `jstp` since there already is a function that is not a part of JSRS. * Fix `binding.gyp`: make `cflags` not ignored on macOS (as it appeared they used to be) and do not use `-O3` in Debug configuration.

belochub · 2017-01-31T00:34:29Z

src/jsrs_serializer.cc

+// string, but always at least by two, even when the function
+// returns nullptr.
+static const char* GetEscapedControlChar(char str, size_t* size) {
+  static const char* control_chars[0x20] = {


According to Google C++ Style Guide it is better to use constexpr instead of const in this situation.

@belochub and get a warning: ISO C++11 does not allow conversion from string literal to 'char *const' 😉

@belochub 330caf9

belochub · 2017-01-31T01:22:00Z

src/jsrs_serializer.cc

+  const char* key_str = *key;
+  for (int i = 0; i < key.length(); i++) {
+    if (key_str[i] == '_') continue;
+    if ((i == 0 && !isalpha(key_str[i])) || !isalnum(key_str[i])) {


Functions std::isalpha and std::isalnum are both defined in <cctype> header which is not included in this file.

Fixed in 07eafcc

belochub · 2017-01-31T01:25:37Z

src/packet_parser.cc

+  int index = 0;
+
+  while (parsed_size < total_size) {
+    auto chunk_size = strlen(curr_chunk);


std::strlen is defined in <cstring> which is not included.

belochub · 2017-01-31T01:58:29Z

src/node_bindings.cc

+  HandleScope scope(isolate);
+
+  String::Utf8Value str(args[0]->ToString());
+  auto array = Local<Array>::Cast(args[1]);


I guess it is better to use v8::Local::As here to keep cast style consistent, but this one is optional and completely up to you.

@belochub a103c2e

aqrln · 2017-01-31T02:27:32Z

@belochub I've added new commits that fix mentioned nits and issues. Does this LGTY?

Since the C headers are included as <clib> instead of <lib.h>, treat them as a part of C++ standard library, move the #include directives to the same group as C++ includes and add `using` statements.

aqrln · 2017-01-31T15:18:48Z

@belochub I don't think we need separate macros to create strings. It was only isolate->ThrowException together with string instantiation thing that created lots of boilerplate code, and that its handled by a single macro, so I mark the last item as completed. What do you think?

belochub · 2017-01-31T15:34:21Z

@aqrln, yes, I think at can be marked as completed. Changes LGTM.

* Split `jsrs-impl.cc` into separate modules. * Make some refactoring. * Rename the native addon to `jstp` since there already is a function that is not a part of JSRS. * Fix `binding.gyp`: make `cflags` not ignored on macOS (as it appeared they used to be) and do not use `-O3` in Debug configuration. * Use a macro to throw V8 exceptions to avoid boilerplate code. PR-URL: #36

aqrln · 2017-01-31T15:44:03Z

Landed in 56c57fe.

* Split `jsrs-impl.cc` into separate modules. * Make some refactoring. * Rename the native addon to `jstp` since there already is a function that is not a part of JSRS. * Fix `binding.gyp`: make `cflags` not ignored on macOS (as it appeared they used to be) and do not use `-O3` in Debug configuration. * Use a macro to throw V8 exceptions to avoid boilerplate code. PR-URL: #36

* Split `jsrs-impl.cc` into separate modules. * Make some refactoring. * Rename the native addon to `jstp` since there already is a function that is not a part of JSRS. * Fix `binding.gyp`: make `cflags` not ignored on macOS (as it appeared they used to be) and do not use `-O3` in Debug configuration. * Use a macro to throw V8 exceptions to avoid boilerplate code. Backport-of: #36

aqrln · 2017-04-02T13:50:26Z

@belochub do you know why was this marker as minor and not patch?

belochub · 2017-04-02T13:57:45Z

@aqrln, I don't remember the actual reason.

aqrln · 2017-04-02T13:59:41Z

Ah, doesn't matter for now. Minor and patch are the same till 1.0. Marked the backport PR as minor though too.

* Split `jsrs-impl.cc` into separate modules. * Make some refactoring. * Rename the native addon to `jstp` since there already is a function that is not a part of JSRS. * Fix `binding.gyp`: make `cflags` not ignored on macOS (as it appeared they used to be) and do not use `-O3` in Debug configuration. * Use a macro to throw V8 exceptions to avoid boilerplate code. Backport-of: #36

* Split `jsrs-impl.cc` into separate modules. * Make some refactoring. * Rename the native addon to `jstp` since there already is a function that is not a part of JSRS. * Fix `binding.gyp`: make `cflags` not ignored on macOS (as it appeared they used to be) and do not use `-O3` in Debug configuration. * Use a macro to throw V8 exceptions to avoid boilerplate code. Backport-of: #36 PR-URL: #110

* Split `jsrs-impl.cc` into separate modules. * Make some refactoring. * Rename the native addon to `jstp` since there already is a function that is not a part of JSRS. * Fix `binding.gyp`: make `cflags` not ignored on macOS (as it appeared they used to be) and do not use `-O3` in Debug configuration. * Use a macro to throw V8 exceptions to avoid boilerplate code. PR-URL: #36

This is a new and shiny first major release for `metarhia-jstp`. Changes include API refactoring and improvements, implementations of CLI, sessions, and application versions, native addon build optimizations, lots of bug fixes, test coverage increase, and other, less notable changes. This release also denotes the bump of the protocol version to v1.0. The only difference from the previous version of the protocol is that "old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong` messages must be used for this purpose instead. Notable changes: * **src,build:** improve the native module subsystem *(Alexey Orlenko)* [#36](#36) **\[semver-minor\]** * **build:** compile in ISO C++11 mode *(Alexey Orlenko)* [#37](#37) **\[semver-minor\]** * **build:** improve error handling *(Alexey Orlenko)* [#40](#40) **\[semver-minor\]** * **lib:** refactor record-serialization.js *(Alexey Orlenko)* [#41](#41) **\[semver-minor\]** * **protocol:** change the format of handshake packets *(Alexey Orlenko)* [#54](#54) **\[semver-major\]** * **parser:** remove special case for '\0' literal *(Mykola Bilochub)* [#68](#68) **\[semver-major\]** * **client:** drop redundant callback argument *(Alexey Orlenko)* [#104](#104) **\[semver-major\]** * **client:** handle errors in connectAndInspect *(Alexey Orlenko)* [#105](#105) **\[semver-major\]** * **socket,ws:** use socket.destroy() properly *(Alexey Orlenko)* [#84](#84) **\[semver-major\]** * **cli:** add basic implementation *(Mykola Bilochub)* [#107](#107) **\[semver-minor\]** * **connection:** fix error handling in optional cbs *(Alexey Orlenko)* [#147](#147) **\[semver-major\]** * **lib:** change event signature *(Denys Otrishko)* [#187](#187) **\[semver-major\]** * **lib:** add address method to Server *(Denys Otrishko)* [#190](#190) **\[semver-minor\]** * **lib:** optimize connection events *(Denys Otrishko)* [#222](#222) **\[semver-major\]** * **lib:** refactor server and client API *(Denys Otrishko)* [#209](#209) **\[semver-major\]** * **lib,src:** rename term packet usages to message *(Denys Otrishko)* [#270](#270) **\[semver-major\]** * **lib:** emit events about connection messages *(Denys Otrishko)* [#252](#252) **\[semver-minor\]** * **connection:** make callback method private *(Alexey Orlenko)* [#306](#306) **\[semver-major\]** * **lib:** implement sessions *(Mykola Bilochub)* [#289](#289) **\[semver-major\]** * **connection:** use ping-pong instead of heartbeat *(Dmytro Nechai)* [#303](#303) **\[semver-major\]**

This is a new and shiny first major release for `metarhia-jstp`. Changes include API refactoring and improvements, implementations of CLI, sessions, and application versions, native addon build optimizations, lots of bug fixes, test coverage increase, and other, less notable changes. This release also denotes the bump of the protocol version to v1.0. The only difference from the previous version of the protocol is that "old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong` messages must be used for this purpose instead. Notable changes: * **src,build:** improve the native module subsystem *(Alexey Orlenko)* [#36](#36) **\[semver-minor\]** * **build:** compile in ISO C++11 mode *(Alexey Orlenko)* [#37](#37) **\[semver-minor\]** * **build:** improve error handling *(Alexey Orlenko)* [#40](#40) **\[semver-minor\]** * **lib:** refactor record-serialization.js *(Alexey Orlenko)* [#41](#41) * **parser:** fix a possible memory leak *(Alexey Orlenko)* [#44](#44) **\[semver-minor\]** * **protocol:** change the format of handshake packets *(Alexey Orlenko)* [#54](#54) **\[semver-major\]** * **parser:** make parser single-pass *(Mykola Bilochub)* [#61](#61) * **parser:** remove special case for '\0' literal *(Mykola Bilochub)* [#68](#68) **\[semver-major\]** * **parser:** fix bug causing node to crash *(Mykola Bilochub)* [#75](#75) * **client:** drop redundant callback argument *(Alexey Orlenko)* [#104](#104) **\[semver-major\]** * **client:** handle errors in connectAndInspect *(Alexey Orlenko)* [#105](#105) **\[semver-major\]** * **socket,ws:** use socket.destroy() properly *(Alexey Orlenko)* [#84](#84) **\[semver-major\]** * **cli:** add basic implementation *(Mykola Bilochub)* [#107](#107) **\[semver-minor\]** * **connection:** fix error handling in optional cbs *(Alexey Orlenko)* [#147](#147) **\[semver-major\]** * **test:** add JSON5 specs test suite *(Alexey Orlenko)* [#158](#158) * **lib:** change event signature *(Denys Otrishko)* [#187](#187) **\[semver-major\]** * **lib:** add address method to Server *(Denys Otrishko)* [#190](#190) **\[semver-minor\]** * **parser:** implement NaN and Infinity parsing *(Mykola Bilochub)* [#201](#201) * **parser:** improve string parsing performance *(Mykola Bilochub)* [#220](#220) * **lib:** optimize connection events *(Denys Otrishko)* [#222](#222) **\[semver-major\]** * **lib:** refactor server and client API *(Denys Otrishko)* [#209](#209) **\[semver-major\]** * **lib,src:** rename term packet usages to message *(Denys Otrishko)* [#270](#270) **\[semver-major\]** * **lib:** emit events about connection messages *(Denys Otrishko)* [#252](#252) **\[semver-minor\]** * **lib:** implement API versioning *(Denys Otrishko)* [#231](#231) **\[semver-minor\]** * **lib:** allow to set event handlers in application *(Denys Otrishko)* [#286](#286) **\[semver-minor\]** * **lib:** allow to broadcast events from server *(Denys Otrishko)* [#287](#287) **\[semver-minor\]** * **connection:** make callback method private *(Alexey Orlenko)* [#306](#306) **\[semver-major\]** * **lib:** implement sessions *(Mykola Bilochub)* [#289](#289) **\[semver-major\]** * **connection:** use ping-pong instead of heartbeat *(Dmytro Nechai)* [#303](#303) **\[semver-major\]**

* Split `jsrs-impl.cc` into separate modules. * Make some refactoring. * Rename the native addon to `jstp` since there already is a function that is not a part of JSRS. * Fix `binding.gyp`: make `cflags` not ignored on macOS (as it appeared they used to be) and do not use `-O3` in Debug configuration. * Use a macro to throw V8 exceptions to avoid boilerplate code. PR-URL: metarhia/jstp#36

aqrln requested a review from belochub January 30, 2017 23:48

aqrln self-assigned this Jan 30, 2017

aqrln added parser refactor and removed refactor labels Jan 30, 2017

aqrln added 6 commits January 31, 2017 02:17

src: add missing include to packet_parser.cc

4c1379d

build: use variables to avoid duplicates in config

111dd48

src: use a macro to throw exceptions

e816826

src: enforce consistent style in node_bindings.cc

2224376

src: remove unnecessary using statements

e4ce38b

src: make style fixes and fix cpplint warnings

1046fa1

belochub requested changes Jan 31, 2017

View reviewed changes

aqrln added 3 commits January 31, 2017 04:11

src: add missing headers

07eafcc

src: make static arrays constexpr

330caf9

src: make local handles cast style consistent

a103c2e

src: treat C headers as a part of C++ library

df4efc6

Since the C headers are included as <clib> instead of <lib.h>, treat them as a part of C++ standard library, move the #include directives to the same group as C++ includes and add `using` statements.

belochub approved these changes Jan 31, 2017

View reviewed changes

aqrln changed the title ~~src,build: improve the native module subsystem [WIP]~~ src,build: improve the native module subsystem Jan 31, 2017

aqrln closed this Jan 31, 2017

aqrln deleted the refactor-src branch January 31, 2017 15:46

aqrln added the semver-minor label Jan 31, 2017

aqrln mentioned this pull request Apr 2, 2017

(v0.5 backport) src,build: improve the native module subsystem #110

Closed

belochub mentioned this pull request Jan 22, 2018

v1.0.0 proposal #311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,build: improve the native module subsystem #36

src,build: improve the native module subsystem #36

aqrln commented Jan 30, 2017 •

edited

Loading

belochub Jan 31, 2017

aqrln Jan 31, 2017

aqrln Jan 31, 2017 •

edited

Loading

belochub Jan 31, 2017

aqrln Jan 31, 2017

belochub Jan 31, 2017

aqrln Jan 31, 2017

belochub Jan 31, 2017

aqrln Jan 31, 2017

aqrln commented Jan 31, 2017

aqrln commented Jan 31, 2017

belochub commented Jan 31, 2017

aqrln commented Jan 31, 2017

aqrln commented Apr 2, 2017

belochub commented Apr 2, 2017

aqrln commented Apr 2, 2017

src,build: improve the native module subsystem #36

src,build: improve the native module subsystem #36

Conversation

aqrln commented Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln Jan 31, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln commented Jan 31, 2017

aqrln commented Jan 31, 2017

belochub commented Jan 31, 2017

aqrln commented Jan 31, 2017

aqrln commented Apr 2, 2017

belochub commented Apr 2, 2017

aqrln commented Apr 2, 2017

aqrln commented Jan 30, 2017 •

edited

Loading

aqrln Jan 31, 2017 •

edited

Loading