Skip to content
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

[C++] Cleanup ATNDeserializer interface #3326

Merged
merged 2 commits into from
Nov 28, 2021
Merged

Conversation

jcking
Copy link
Collaborator

@jcking jcking commented Oct 28, 2021

This change does a few things. First it changes ATNDeserializer to cache the well known UUIDs using std::call_once which avoids static initialization issues and after the first call has the overhead of only a single atomic load using acquire semantics. Currently the UUIDs are parsed every invocation of the functions.

Secondly it marks a few things as const and removes the virtual specifiers on ATNDeserializationOptions. This is currently not useful as ATNDeserializationOptions is copied in ATNDeserialization so inheriting from the class doesn't accompolish much. Additionally this allows us to inline a bunch of functions giving the compiler a better chance at optimizing things away. Additionally it uses std::call_once to initialize the default options.

Lastly it throws IllegalStateException when marked as read only, instead of throwing a raw C string which is a big no-no.

Note that static initialization order between compilation units is undefined per the C++ standard. To get around this we use std::call_once and std::unique_ptr which both have constant initialization.

@jcking
Copy link
Collaborator Author

jcking commented Oct 28, 2021

@mike-lischke More cleanup/optimization.

@jcking jcking force-pushed the cpp-guid-cache branch 7 times, most recently from ef8169a to 87acadb Compare October 28, 2021 18:30
@jcking jcking marked this pull request as ready for review October 28, 2021 18:31
@mike-lischke
Copy link
Member

Note: most of the methods which are virtual don't need to be that, but the C++ runtime was made to mimic the Java runtime as close as possible. After maturing we can probably now remove some of the virtual specifiers, but we have to be careful, as users might rely on the virtuality in their projects.

@jcking jcking force-pushed the cpp-guid-cache branch 3 times, most recently from b11bcba to fb23a3c Compare November 1, 2021 13:45
@jcking
Copy link
Collaborator Author

jcking commented Nov 1, 2021

Note: most of the methods which are virtual don't need to be that, but the C++ runtime was made to mimic the Java runtime as close as possible. After maturing we can probably now remove some of the virtual specifiers, but we have to be careful, as users might rely on the virtuality in their projects.

In this instance, I think removing virtual is safe. There was no way of overriding the virtual methods and using them with ATNDeserializer as the options are copied by value and not referenced.

runtime/Cpp/runtime/src/atn/ATNDeserializer.h Outdated Show resolved Hide resolved
runtime/Cpp/runtime/src/atn/ATNDeserializer.cpp Outdated Show resolved Hide resolved
@jcking jcking force-pushed the cpp-guid-cache branch 2 times, most recently from 09ddc76 to 52b7eea Compare November 15, 2021 16:10
@jcking jcking force-pushed the cpp-guid-cache branch 3 times, most recently from da5e341 to 5c4cccc Compare November 15, 2021 20:14
@mike-lischke
Copy link
Member

@parrt Here's a pure C++ patch, ready for merge.

@parrt parrt merged commit 1493859 into antlr:master Nov 28, 2021
@parrt parrt added this to the 4.9.4 milestone Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants