Skip to content

Conversation

@parrt
Copy link
Member

@parrt parrt commented Feb 12, 2022

I generated code with getSerializedATNSegmentLimit() set to 80 (current dev value) and max int. Here is diff with C++ target:

Screen Shot 2022-02-12 at 1 02 24 PM

On the left we see small segments but it's unnecessary and wasteful compared to the big static array on right. This PR makes it look like right side. Only Java really needs this. Targets are free to ignore getSerializedATNSegmentLimit() as JavaScript does. I.e., JavaScript always looks like right side of pic.

@KvanTTT @marcospassos @jcking @ericvergnaud @mike-lischke Any concerns?

@parrt parrt added the code-gen label Feb 12, 2022
@parrt parrt added this to the 4.10 milestone Feb 12, 2022
@KvanTTT
Copy link
Member

KvanTTT commented Feb 12, 2022

It's ok, but I've already fixed it in #3513 Also, C++ does not need getSerializedATNSegmentLimit call at all because it does not have a restriction on the size of array of literal.

@parrt
Copy link
Member Author

parrt commented Feb 12, 2022

Ok, well, maybe take the new comments i have here?

JavaScript does:

SerializedATN(model) ::= <<
<! only one segment, can be inlined !>

const serializedATN = ["<model.serialized; wrap={",<\n>    "}>"].join("");

>>

And C++ can be simplified to the equivalent if someone wants to put the effort in.

@mike-lischke
Copy link
Member

Looks fine to me.

@KvanTTT
Copy link
Member

KvanTTT commented Feb 12, 2022

Ok, well, maybe take the new comments i have here?

Let's merge your PR and I'll rebase on dev and drop my commit.

And C++ can be simplified to the equivalent if someone wants to put the effort in.

Yes, I've already done it as well. Also for Dart and PHP.

@parrt parrt merged commit c3f4197 into antlr:dev Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants