Add well formed JSON.stringify, String.prototype.isWellFormed, and String.prototype.toWellFormed#1628
Conversation
| ~language/expressions/new.target | ||
|
|
||
| language/expressions/object 865/1169 (73.99%) | ||
| language/expressions/object 863/1169 (73.82%) |
There was a problem hiding this comment.
Any idea what's going on here? The stats for updated, but I don't see any tests being unbroken in this section
There was a problem hiding this comment.
Not sure, could a previous PR have fixed something but forgot to update the properties file?
There was a problem hiding this comment.
Well, the last thing that got merged was another one of your PRs 🤔
There was a problem hiding this comment.
Ok no idea then 😅
There was a problem hiding this comment.
I have the impression i have seen something similar for other pr's also, but i might be wrong
There was a problem hiding this comment.
You probably need to rebase this branch. language/expressions/object 863/1169 (73.82%) is the value currently in the master branch. It's not being added with this PR.
| } | ||
| case Id_isWellFormed: | ||
| { | ||
| String str = ScriptRuntime.toString(requireObjectCoercible(cx, thisObj, f)); |
There was a problem hiding this comment.
This is minor but it will likely help with performance in a bunch of cases -- how about using "toCharSequence" here instead of "toString"? That would mean that concatenated strings here don't need to be re-concatenated, and would work fine since it looks like you're just iterating character-by-character anyway.
|
See above, I'm curious if you could use CharSequence here instead for performance, otherwise this looks good and thanks! |
|
I will try that out in a little bit! |
| return product.toString(); | ||
| } | ||
|
|
||
| static boolean isLeadingSurrogate(char c) { |
There was a problem hiding this comment.
Are these checks specific to JSON or more general? If they are not specific to JSON i vote for haing a more central place for these methods. Not sure if we already have one (sorry i only have my cell phone at hand at the moment)
There was a problem hiding this comment.
No they aren't. Not sure where they should go though...
There was a problem hiding this comment.
@gbrail @p-bakker @tonygermano any suggestion?
There was a problem hiding this comment.
I guess it could go into ScriptRuntime? Not sure of a better spot.
| } | ||
| case Id_toWellFormed: | ||
| { | ||
| String str = ScriptRuntime.toString(requireObjectCoercible(cx, thisObj, f)); |
There was a problem hiding this comment.
Maybe this kind of string processing can be optimized even more. Have a first loop that simply traverses the string until you reach a char that has to be replaced. Then create the string builder an use the already processed chars as initial input. Now you can process like you did.
This ends up with more code but avoidas the string buider and the reconstruction of the string in all standard cases. I assume that most of the json to be normalized does not contain surrogates.
There was a problem hiding this comment.
That's a great point, thanks! I can try this out and see how it goes.
camnwalter
force-pushed
the
wellformed
branch
from
c6aab11 to
c6f456e
Compare
|
|
||
| if (NativeJSON.isLeadingSurrogate(prev) | ||
| && NativeJSON.isTrailingSurrogate(c)) { | ||
| surrogates.put(Integer.valueOf(i - 1), Boolean.TRUE); |
There was a problem hiding this comment.
I used the Boxed types for the map because I saw a commit from a few years ago that reduced the amount of auto boxing.
This also adds a shortcut if there are no surrogate characters, to just reuse the existing string instead of creating a new one.
camnwalter
force-pushed
the
wellformed
branch
from
c6f456e to
cc4e73c
Compare
|
This looks good and I'm going to merge it. Thanks! |
5 participants
Closes #982