-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed ANTLRInputStream and ANTLRFileStream #3113
Merged
Merged
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
e24f97d
Merge branch 'master-upstream'
mike-lischke 8391136
Merge branch 'master-upstream'
mike-lischke 62a829f
Fixed ANTLRInputStream and ANTLRFileStream
mike-lischke faa64fd
Merge branch 'master-upstream'
mike-lischke 93c0621
Fixed C++ tests and build warnings
mike-lischke f881e3e
Wrong load call fixed.
mike-lischke 84d4ce7
More changes to the ANTLRInputStream and ANTLRFileStream classes
mike-lischke 5731e64
Added a sanity check for input size.
mike-lischke 9d1737f
Build fix
mike-lischke ff629d5
Another build fix
mike-lischke 4431f1f
Small improvement
mike-lischke File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
runtime/Cpp/runtime/antlrcpp.xcodeproj/xcshareddata/xcschemes/antlr4.xcscheme
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm just nitpicking at this point but wouldn't an empty bom be a valid empty string that could be valid for some grammars? So the check would be
length >= 3
to accept and skip if it's just the bom. Or maybe I don't know my unicode. Sorry for all the disturbance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, a good code review is often inconvenient.
The approach there is that if there's enough to have a possible BOM then check it, if not just go ahead and load what you got. So even an empty string is "loaded" correctly in the else branch.
I'm assuming here that by "empty bom" you actually mean an empty input string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I mean: if only
"\xef\xbb\xbf"
is passed in the function,length
will be 3 and theelse
branch will be taken. Then, at least when used with utfcpp conversion functions, it will throwutf8::invalid_code_point
because it can't parse the bom.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also thought about this case and found it would not be worth to be considered as it would mean no useful input was given. However, there could be a workflow where a BOM is always attached automatically, regardless of what input was given and empty input is often valid input. So it would make sense to strip off the BOM and deal with the empty input anyway. I changed the check therefore.