Skip to content

Fixing reserve word NULL for cpp targets #3889

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Oct 13, 2022
Merged

Fixing reserve word NULL for cpp targets #3889

merged 21 commits into from
Oct 13, 2022

Conversation

@1sand0s
Copy link
Contributor

@1sand0s 1sand0s commented Sep 15, 2022 •
edited
Loading

PR Summary

  1. The ANTLR grammar for Cypher contains a Token named NULL which is a reserve word in Cpp. This PR adds NULL to the list of reserve words in CppTarget.java.
  2. I passed the String arguments of getTargetStringLiteralFromString and getTargetStringLiteralFromANTLRStringLiteral in Target.java, s and literal respectively, to Target's escapeIfNeeded function since potential reserve words must be handled before being accumulated into the StringBuilders
  3. I passed the Key of g.tokenNameToTypeMap in Recognizer to Target's escapeIfNeeded function before being inserted into tokens in Recognizer

Best

@1sand0s 1sand0s changed the title Fixing reserve word null for cpp targets Fixing reserve word NULL for cpp targets Sep 15, 2022
KvanTTT
KvanTTT suggested changes Sep 18, 2022
View reviewed changes
Copy link
Member

@KvanTTT KvanTTT left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add new or extend the existing test ReservedWordsEscaping.txt that covers suggested changes.

@1sand0s 1sand0s force-pushed the Fixing_Reserve_Word_NULL_for_Cpp_Targets branch 2 times, most recently from bf5e884 to b20db8c Compare September 23, 2022 07:44
KvanTTT
KvanTTT reviewed Sep 23, 2022
View reviewed changes
Jim.Idle and others added 13 commits September 23, 2022 16:32
@1sand0s
fix: Restore missing changes to v4 of go runtime
531654e 
I had not though about this, but I guess some of the fix PRs did not have one fix
that was in the legacy version of the runtime, copied in to the v4 branch. This
commit fixes that.

Signed-off-by: Jim.Idle <[email protected]>
Signed-off-by: 1sand0s <[email protected]>
@parrt @1sand0s
tweak spelling errors in Interpreter
9e96de8 
Signed-off-by: Terence Parr <[email protected]>
Signed-off-by: 1sand0s <[email protected]>
@parrt @1sand0s
Describe using antlr4-tools in getting started documentation
3dacdff 
Signed-off-by: Terence Parr <[email protected]>
Signed-off-by: 1sand0s <[email protected]>
@parrt @1sand0s
Tweak doc
7a06e1f 
Signed-off-by: Terence Parr <[email protected]>
Signed-off-by: 1sand0s <[email protected]>
@ericvergnaud @1sand0s
Update C# release instructions
253ef6b 
Signed-off-by: 1sand0s <[email protected]>
@parrt @1sand0s
Add Windows doc for antlr4-tool usage
871cf52 
Signed-off-by: Terence Parr <[email protected]>
Signed-off-by: 1sand0s <[email protected]>
@1sand0s
Fixing minimum CXX standard required
e20088f 
Signed-off-by: 1sand0s <[email protected]>
@1sand0s
Fixing Reserve word NULL handling for Cpp targets
6ed94f8 
Signed-off-by: 1sand0s <[email protected]>
@1sand0s
Adding testcase for reserve word NULL for Cpp targets
82beced 
Signed-off-by: 1sand0s <[email protected]>
@1sand0s
Update ReservedWordsEscaping_NULL.txt
e347ffe 
Signed-off-by: 1sand0s <[email protected]>
@1sand0s
Fixing Reserve word NULL handling for Cpp targets
57cb224 
Signed-off-by: 1sand0s <[email protected]>
@1sand0s
Adding testcase for reserve word NULL for Cpp targets
61f6aea 
Signed-off-by: 1sand0s <[email protected]>
@1sand0s
Removing unecessary escape
d26c064 
Signed-off-by: 1sand0s <[email protected]>
@1sand0s 1sand0s force-pushed the Fixing_Reserve_Word_NULL_for_Cpp_Targets branch from 515dc16 to d26c064 Compare September 23, 2022 21:35
@KvanTTT
Copy link
Member

KvanTTT commented Sep 24, 2022 •
edited
Loading

Could you please get rid of unrelated commits (using interactive rebase)? Some changes are unrelated to the topic.

1sand0s and others added 7 commits September 26, 2022 11:14
@1sand0s
Copy link
Contributor Author

1sand0s commented Sep 26, 2022

Could you please get rid of unrelated commits (using interactive rebase)? Some changes are unrelated to the topic.

@KvanTTT Done !

@1sand0s
Copy link
Contributor Author

1sand0s commented Oct 1, 2022

Could you please get rid of unrelated commits (using interactive rebase)? Some changes are unrelated to the topic.

@KvanTTT Just a reminder that I have removed unrelated commits.

Thanks !

@KvanTTT
Copy link
Member

KvanTTT commented Oct 4, 2022

I see unrelated commits anyway. @parrt the changes look good to me, but I suggest squashing all commits since most of them are useless.

parrt
parrt reviewed Oct 5, 2022
View reviewed changes
parrt
parrt reviewed Oct 5, 2022
View reviewed changes
@parrt
Copy link
Member

parrt commented Oct 9, 2022

Looks like we have a merge conflict. Can you take a look / rebase?

@parrt parrt added this to the 4.11.2 milestone Oct 9, 2022
@1sand0s
Copy link
Contributor Author

1sand0s commented Oct 10, 2022 •
edited
Loading

Looks like we have a merge conflict. Can you take a look / rebase?

@parrt I compared the antlr4/dev branch with 1sand0s/antlr4/Fixing_Reserve_Word_NULL_for_Cpp_Targets branch and did not find merge conflicts on my end.

@parrt parrt merged commit d685f7b into antlr:dev Oct 13, 2022
@parrt
Copy link
Member

parrt commented Oct 13, 2022

AH. it was set at rebase not merge. A merge will work. thanks!

@admin-token-bot admin-token-bot mentioned this pull request Jul 7, 2024
Closed
@ThatGhostToast ThatGhostToast mentioned this pull request Nov 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@parrt parrt parrt left review comments

+1 more reviewer

@KvanTTT KvanTTT KvanTTT requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

grammars target:cpp type:cleanup

Projects

None yet

Milestone

4.12.0

Development

Successfully merging this pull request may close these issues.

4 participants

@1sand0s @KvanTTT @parrt @ericvergnaud