Complete rewrite of CBA_fnc_parseJSON #1582
Conversation
initramfs
force-pushed
the
rewrite-parse-json
branch
from
5118207
to
8148521
Compare
|
Grew by almost 300 lines, more than doubled. Not a fan of try/throw/catch in SQF. Does it still pass test_parseJSON.sqf? |
initramfs
force-pushed
the
rewrite-parse-json
branch
from
394eca0
to
2dca726
Compare
|
@commy2 As stated in my sidenotes I wrote this parser for my own mod, as I am not on the CBA team I cannot gauge if the increase in complexity is worth the functionality/feature improvement. I would like to point out however that the original implementation does not fulfill it's contractual obligations in the preamble:
There are quite a few cases where the script will error out due to invalid parameters being passed into scripting commands (e.g. into My motivation for writing my own parser came from my frustration when debugging a script that I wrote where I spent way too long trying to pinpoint the issue down, never expecting it to be from a CBA function (see PR #1580). With respect to the usage of SQF exceptions, I don't really see any other way to pass information back to the caller in a structured way without further API changes (e.g. changing the return value to a n-tuple [array] representing if the parsing was successful + the JSON object [if successful] or error message and line numbers [if unsuccessful]). This is especially true given the output of this function could be almost all data types SQF supports (especially when you consider the additional The exception type being thrown is always a string in this case but I can always change it to something more structured (e.g. array or hashmap) if the caller wants to be able to structurally parse the exception object. As an additional note: as stated above the current implementation of the parser can cause runtime errors that are uncatchable (given those are errors from invalid parameters to the scripting engine) which is, in my opinion, worse than exceptions. Of course another solution to this entire issue would be to force in the preconditions of the function that the JSON string must always be valid, though I would argue that represents the largest API change/deviation among all the ones I have mentioned. With respect to the unit tests: So it appears it still passes all the unit tests. |
|
CBA never uses exceptions. I get the possibilities compared to just using the ERROR_N macro and returning nil, but meh. Probably needs to think about what value to throw, because that would become part of the public API. The helper function part should be simplified and optimized to: private ["_createObject", "_objectSet"];
switch (_objectType) do {
case false;
case 0: {
_createObject = {[] call CBA_fnc_createNamespace};
_objectSet = {
params ["_obj", "_key", "_val"];
_obj setVariable [_key, _val];
};
};
case true;
case 1: {
_createObject = {[] call CBA_fnc_hashCreate};
_objectSet = {
params ["_obj", "_key", "_val"];
[_obj, _key, _val] call CBA_fnc_hashSet;
};
};
case 2: {
_createObject = {createHashMap};
_objectSet = {
params ["_obj", "_key", "_val"];
_obj set [_key, _val];
};
};
};No need to look up _objectType repeatedly when it never changes. |
addons/hashes/fnc_parseJSON.sqf
Outdated
| forceUnicode 0; | ||
|
|
||
| /** | ||
| * Helper function returning the code used for object creation based on the "_objectType" parameter. |
There was a problem hiding this comment.
Comment explains what the code does. That's just duplication and not useful. If you want to keep it as separator, just use // helper function.
There was a problem hiding this comment.
I've removed the small helper functions with the original code so there are no additional function declarations anymore. The original rationale for the comment is merely my codestyle of always having a preamble for any function declaration.
I believe the preambles for _tokenize, _reduce definitely provide additional information (wrt. output format, preconditions, etc). To a lesser extent _shift and _parse do as well, and it seems a little inconsistent to me for some functions to get a full descriptor and some to not.
I've essentially restored what the original code did which does a single check and sets With respect to exceptions, this is the first piece of CBA code that I have touched so I'm not familiar with what kind of public API is expected (for example, I didn't know that there were no CBA functions that threw exceptions). As for what type of object to throw on parse error, I really don't have any input as to what is appropriate, I simply wrote something that was appropriate for myself. In essence, I really don't have any stake in the matter and I can change the exception (and the exception documentation) to anything you want (or even just try/catch and return I suggest perhaps discussion with other CBA maintainers for input, or if you're in a position to make decisions yourself, just tell me what you want me to do. |
Idk either, because there was never any need to do it. Just saying that we should think about what is useful, because changing it later could break stuff.
They may speak below: |
|
While I see the goals and pros of this approach, I dislike the use of try/catch/throw in SQF and the significant increase in codebase and complexity with that. @BaerMitUmlaut Would be interesting to see your take on that. |
|
If you want to know my opinion, this is way too complex, the function is now twice the size. I don't like full rewrites in general. There's also no tests for the new functionality. |
Well we have 2.14 now ;) |
As compared to my last PR (#1580) this represents a complete rewrite of the JSON parsing code using the latest features available (as of the time of writing: version 2.12) in the scripting engine (though heavily inspired by the original shift-reduce design).
The parser is now (assuming bug-free) a fully validating parser that supports all the grammar defined at json.org, including previously unsupported features such as hex escape strings.
In addition to simply parsing a JSON document it can also track row/column numbers of the input tokens in order to generate appropriate error messages upon encountering an invalid JSON document. Row/column numbers are tracked at the unicode code point level rather than at the actual grapheme level for simplicity (so the use of things like
U+200D ZERO WIDTH JOINERmay cause an offset error in the reported column index). Row numbers are defined strictly by the presence of the code pointU+000A LINE FEEDso they should always be correct (technically there exists documents out there that exclusively useU+000D CARRIAGE RETURNor other exotic characters to denote new lines but catering to this would represent an unreasonable increase complexity that I was not willing to write).This parser (as it's currently written) does represent an API change for
CBA_fnc_parseJSONas instead of returningnilon error it throws an exception with an appropriate string message (though the old behaviour can be easily restored by wrapping the internal parse function call in a try-catch, this will effectively eliminate the purpose of the row/column tracking feature however).When merged this pull request will:
CBA_fnc_parseJSONinto a fully validating parsing function to deserialize objects from JSON strings, throwing exceptions at any parsing errors (including row/column numbers and error reasoning).Side notes: