-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Parse] Introduce /.../
regex literals
#42119
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
07b3c76
[DiagnosticEngine] Introduce DiagnosticQueue
hamishknight 080d59b
[Lexer] Delay token diagnostics
hamishknight 63b8db1
Start using '-enable-bare-slash-regex'
hamishknight 5a8dff0
[Parse] Emit error on prefix operator containing `/`
hamishknight 9f384d3
[Lexer] Remove `r'...'` lexing logic
hamishknight f1a7990
[Parse] Introduce `/.../` regex literals
hamishknight 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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
Oops, something went wrong.
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.
Let me propose an alternate design for
DiagnosticQueue
:UnderlyingEngine
memberDiagnosticQueue::DiagnosticQueue()
just adds aForwardingDiagnosticConsumer
toQueueEngine
emit()
just closes theQueueEngine
's transaction, flushing the tentative diagnostics out to consumersI think that would mean you wouldn't have to modify
DiagnosticEngine
at all. Does your design have advantages over that one? (It might—I'd like to hear about them.)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.
So I did play about with
ForwardingDiagnosticConsumer
a bit, the main issue is that is forwards directly to the consumers and bypasses the target engine altogether, so doesn't update any state on the engine (includinghadAnyError
!). Now, we should definitely fix that, although it's not clear to me whether all of its clients actually want the diagnostic to be fully re-evaluated by the target engine. There are a couple of other things:DiagnosticInfo
back into aDiagnostic
to be re-evaluated by the target engine (as it may need to become a tentative diagnostic). We'd need special handling to make sure e.g warning-as-error is preserved if the original diagnostic engine had it set.ASTContext
or in the evaluator).I don't think there's anything here that can't be solved, but it seemed less clean to me than creating a dummy diagnostic engine to effectively serve as a
Diagnostic
builder, which can then be passed onto a target engine. If we wantForwardingDiagnosticConsumer
to properly support being able to forward-and-re-evaluate a diagnostic, then I agree we should change this to do that. But either way we'll needDiagnosticEngine
changes I believe.