parser: implement NaN and Infinity parsing #201

belochub · 2017-06-01T15:35:21Z

Also fix not throwing on some of the incorrect number inputs.

Refs: #175

nechaido

LGTM

lundibundi

Code looks good overall but there is some unintended behavior:

Input	result
N	0
Na	0
N1222	0
I	0
In	0
In100%	0
Inf100%	Infinity
In42	0
Inf	Infinity (this may be okay)

aqrln · 2017-06-02T21:48:45Z

@lundibundi +1

aqrln · 2017-06-02T21:49:11Z

src/jsrs_parser.cc

@@ -150,6 +151,11 @@ static bool GetType(const char* begin, const char* end, Type* type) {
      }
      break;
    }
+    case 'N':
+    case 'I': {
+      *type = Type::kNumber;


Not sure about this for the reasons @lundibundi has mentioned...

@aqrln, why'd you put an ellipsis in the end of your comment?
Also, the problem stated by @lundibundi has nothing to do with this line, it is just incorrect usage of the strtol function, and I will fix this.

Oops, I am sorry, I was really talking about atof in the previous comment, not strtol.
strtod has to be used instead of it.

aqrln

LGTM

Also fix not throwing on some of the incorrect number inputs. Refs: #175

lundibundi

LGTM with a few nits.

lundibundi · 2017-06-03T12:07:51Z

src/jsrs_parser.cc

+  // strictly allow only "NaN" and "Infinity"
+  if (isnan(number)) {
+    if (strncmp(begin + 1, "aN", 2) != 0) {
+      THROW_EXCEPTION(SyntaxError, "Invalid format: expected NaN");


I think it's better to say 'Invalid number format: ...'.

I wrote it like that to be consistent with the other error messages, it still isn't perfect everywhere, but hopefully I will fix it someday (see #69).

What I wanted to say is that it is an issue for another PR which will improve error reporting in the native parser.

@belochub Ok, let's hope it will be done eventually =)

lundibundi · 2017-06-03T12:08:07Z

src/jsrs_parser.cc

+
+  // strictly allow only "NaN" and "Infinity"
+  if (isnan(number)) {
+    if (strncmp(begin + 1, "aN", 2) != 0) {


wow, such a pain =(

What's so painful here? I just avoided testing for the same character twice here, that's it.

The need to actually check this. But anyway I see no other way.

lundibundi · 2017-06-03T12:08:17Z

src/jsrs_parser.cc

+    }
+  } else if (isinf(number)) {
+    if (strncmp(begin + 1, "nfinity", 7) != 0) {
+      THROW_EXCEPTION(SyntaxError, "Invalid format: expected Infinity");


Also fix not throwing on some of the incorrect number inputs. Refs: #175 PR-URL: #201 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

belochub · 2017-06-03T12:37:28Z

Landed in 8e87a28.

Also fix not throwing on some of the incorrect number inputs. Refs: #175 PR-URL: #201 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

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\]**

Also fix not throwing on some of the incorrect number inputs. Refs: metarhia/jstp#175 PR-URL: metarhia/jstp#201 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

belochub added the parser label Jun 1, 2017

belochub requested a review from aqrln June 1, 2017 15:35

nechaido approved these changes Jun 1, 2017

View reviewed changes

belochub mentioned this pull request Jun 1, 2017

Make number parsing in native parser implementation JSON5 compliant #175

Closed

2 tasks

lundibundi reviewed Jun 2, 2017

View reviewed changes

aqrln reviewed Jun 2, 2017

View reviewed changes

belochub requested review from aqrln and lundibundi June 2, 2017 23:24

aqrln approved these changes Jun 3, 2017

View reviewed changes

parser: implement NaN and Infinity parsing

bcab9a6

Also fix not throwing on some of the incorrect number inputs. Refs: #175

belochub force-pushed the native-parser-numbers branch from d512006 to bcab9a6 Compare June 3, 2017 11:53

lundibundi approved these changes Jun 3, 2017

View reviewed changes

belochub closed this Jun 3, 2017

belochub deleted the native-parser-numbers branch July 21, 2017 01:19

belochub mentioned this pull request Jan 22, 2018

v1.0.0 proposal #311

Merged

aqrln mentioned this pull request Feb 12, 2019

Make the parser compliant with JSON5 specs metarhia/mdsf#40

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parser: implement NaN and Infinity parsing #201

parser: implement NaN and Infinity parsing #201

belochub commented Jun 1, 2017 •

edited

Loading

nechaido left a comment

lundibundi left a comment •

edited

Loading

aqrln commented Jun 2, 2017

aqrln Jun 2, 2017

belochub Jun 2, 2017

belochub Jun 2, 2017

aqrln left a comment

lundibundi left a comment

lundibundi Jun 3, 2017

belochub Jun 3, 2017

belochub Jun 3, 2017

lundibundi Jun 3, 2017

lundibundi Jun 3, 2017

belochub Jun 3, 2017 •

edited

Loading

lundibundi Jun 3, 2017 •

edited

Loading

lundibundi Jun 3, 2017

belochub commented Jun 3, 2017

parser: implement NaN and Infinity parsing #201

parser: implement NaN and Infinity parsing #201

Conversation

belochub commented Jun 1, 2017 • edited Loading

nechaido left a comment

Choose a reason for hiding this comment

lundibundi left a comment • edited Loading

Choose a reason for hiding this comment

aqrln commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln left a comment

Choose a reason for hiding this comment

lundibundi 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

Choose a reason for hiding this comment

belochub Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

lundibundi Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belochub commented Jun 3, 2017

belochub commented Jun 1, 2017 •

edited

Loading

lundibundi left a comment •

edited

Loading

belochub Jun 3, 2017 •

edited

Loading

lundibundi Jun 3, 2017 •

edited

Loading