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.
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
clp-s: Add boilerplate for new sql parser #504
base: main
Are you sure you want to change the base?
clp-s: Add boilerplate for new sql parser #504
Changes from 2 commits
b41eed1
86acbf5
93cb37c
b5f55d3
d96505e
436c396
4037d5b
2b5db82
f8d67fb
ce70d88
0f768a5
a1d8fe7
1805505
4af7f86
d09e6c4
991ca73
ce13759
5696ac3
712afbe
c2432bb
cd7f164
d687fe4
8f00de2
29f8744
f0118f1
866dcfc
e17ebd0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can we add a comment explaining this is a boilerplate? Asked Kirk to take a brief view and his feedback is this file can be confusing to people new to it without a comment explaining it's a WIP config
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.
Fwiw, the Rabbit agrees with me, lol.
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.
yeah it seems like we already have this class in
clp_s::search::kql
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.
Should be removed right?
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.
Sorry to miss this in the last round of review. Shall we move these two classes into the anon namespace? Or if there's plan to reuse them in other places, we should probably move them into the dedicated headers.
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.
I'll move the ErrorListener into its own header, and put the visitor classes for sql and kql into the anonymous namespace.
I was worried that BaseErrorListener was part of the generated code for each grammar but it's part of the generic antlr runtime so there shouldn't be any issues.
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.
Not used?
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.
Its use is entirely implemented in the constructor/destructor. It just turns off spdlog logging once created, and re-enables it once destroyed.
Just allows you to run parsing unit tests without emitting tons of log messages for parsing failure.