Detect comments #42
Detect comments #42
Conversation
new routine that consumes whitespaces and comments. activated by JSON11_COMMENTS pre-processor flag.
|
Automated message from Dropbox CLA bot @capitalaslash, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here and update the thread so we can consider merging your code. |
|
CLA signed |
|
A few design-level points for discussion:
I'll make a few inline comments on individual lines too. I wonder what @j4cbo might think of this too. |
| * | ||
| * Advance until the current character is non-whitespace and non-comment. | ||
| */ | ||
| void consume_garbage() { |
There was a problem hiding this comment.
I feel like a loop would be a safer approach than recursion for consuming more comments/whitespace, since that would avoid the risk that your stack frame grows (and could overflow) when parsing many comments. I feel like this function is the right place to put a loop. If consume_whitespace() and consume_comment() returned a bool to indicate whether they consumed anything (or you just looked at the value of i) it would be easy to decide when to end the loop.
|
Oh, also, welcome, and thanks for your contribution! Forgot to say that in my diving straight into business. :) |
|
cumulative reply to your points:
|
test also for nested and mixed comments. whitespaces/newlines are already intermixed between comments.
add 3 testes for: - unended multi-line comment, - malformed single-line comment, - trailing slash
|
I went for a bool argument with the default set to false, that is quite opaque. |
| bool comment_found = false; | ||
| if (str[i] == '/') { | ||
| i++; | ||
| if (str[i] == '/') { // inline comment |
There was a problem hiding this comment.
I think you need a size check here, and indeed after every increment of i. Otherwise if the string ends with '/' you'd crash. That would be a good negative case to add to your utests.
(Update, this also might not crash due to the null terminator, but I think that's undefined behavior, so should be avoided.)
|
I added a few more inline comments, primarily about some issues with size checks which I didn't see on my first look. I agree with your concern about the opacity of a boolean argument. One approach I've used to avoid that in the past is an enum. You can declare an enum with values like JsonParse::STANDARD and JsonParse::COMMENTS, and pass one or the other instead of the boolean, to make the intent explicit. That approach also can eventually grow to allow enum values to be combined like bitfields, but I wouldn't worry too much about supporting that preemptively. |
|
should have addressed all your concerns. |
| @@ -338,6 +338,7 @@ struct JsonParser { | |||
| size_t i; | |||
| string &err; | |||
| bool failed; | |||
| JsonParse strategy; | |||
| if (i == str.size()) | ||
| return fail("unexpected end of input inside multi-line comment", 0); | ||
| // advance until closing tokens | ||
| while (!(str[i] == '*' && str[i+1] == '/')) { |
There was a problem hiding this comment.
str[i+1] here is still an unchecked value. Should check against size-1 above, and below, since it takes at least 2 characters to terminate a multi-line comment.
|
Looks good. Just one off-by-one check to point out. |
|
ok, fixed. |
|
Thanks! Merging. |
I know that comments are not defined in json standard, but many parsers do support them, and I find them useful.
the patch is activated by a preprocessor macro and the original behavior can be restored by commenting its definition.