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

Typescript target #4027

Merged
merged 180 commits into from
Dec 21, 2022
Merged

Typescript target #4027

merged 180 commits into from
Dec 21, 2022

Conversation

ericvergnaud
Copy link
Contributor

Candidate PR for the typescript target.

hs-apotell and others added 30 commits September 9, 2022 17:09
When using variables to compare (like in if clause) the variable
shouldn't be quoted. More details can be found at the link below:

https://cmake.org/cmake/help/latest/command/if.html#variable-expansion

Signed-off-by: HS <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: nicksxs <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Terence Parr <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Terence Parr <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Terence Parr <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Terence Parr <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Add static library distribution in SPM

Signed-off-by: Hell_Ghost <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Hell_Ghost [email protected]
Signed-off-by: Hell_Ghost <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
cpp builds using actions/ccache.

Builds are more reliable (avoids the archive.apache server which
intermittently reports timeouts) and also significantly improves the
overall builds times (down from 46 mins to 28 mins).

The slowest part of the build now is the Windows+cpp builds because
there is no reliable cache implementation yet. MacOS+cpp (65% cache hit) is
also relatively slow compared to Ubuntu+cpp (99% cache hit).

Signed-off-by: HS <[email protected]>
Signed-off-by: Terence Parr <[email protected]>

# Conflicts:
#	.github/workflows/hosted.yml
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Terence Parr <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
Signed-off-by: Terence Parr <[email protected]>
Signed-off-by: Eric Vergnaud <[email protected]>
@Codex-
Copy link
Contributor

Codex- commented Dec 19, 2022

The types in this PR are not 'generated', they are hand coded

Apologies for the confusion, by generated I'm referring to the output generated by antlr when processing a grammar.

Currently, we use the types package and then use typescript to infer and generate .d.ts files for the Parser, Listener, and Lexer that are generated for the Javascript runtime.

Comment on lines 13 to 14
: e '*' e {$v = <Cast("BinaryContext","$ctx"):ContextMember({<Production("e")>(0)}, {<Result("v")>})> * <Cast("BinaryContext","$ctx"):ContextMember({<Production("e")>(1)}, {<Result("v")>})>;} # binary
| e '+' e {$v = <Cast("BinaryContext","$ctx"):ContextMember({<Production("e")>(0)}, {<Result("v")>})> + <Cast("BinaryContext","$ctx"):ContextMember({<Production("e")>(1)}, {<Result("v")>})>;} # binary
: e '*' e {$v = <Cast("BinaryContext","$ctx"):SubContextLocal({<Production("e")>(0)}, {<Result("v")>})> * <Cast("BinaryContext","$ctx"):SubContextLocal({<Production("e")>(1)}, {<Result("v")>})>;} # binary
| e '+' e {$v = <Cast("BinaryContext","$ctx"):SubContextLocal({<Production("e")>(0)}, {<Result("v")>})> + <Cast("BinaryContext","$ctx"):SubContextLocal({<Production("e")>(1)}, {<Result("v")>})>;} # binary
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, but I don't understand why the changes related to test data are placed into the current pull request related to TypeScript. In my opinion they should be moved into another pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are part of this PR because they were required to make the typescript target tests runnable.
They don’t add any value in themselves, it's simply a technical change to enable correct code generation of typescript tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other words, they're not worth a PR unless we integrate the TypeScript target, so it makes sense to have them with this PR

Comment on lines 326 to 330
ContextListFunction(ctx, rule) ::= "<ctx>.<rule>()"
StringType() ::= "String"
ContextMember(ctx, subctx, member) ::= "<ctx>.<subctx>.<member>"
ContextMember(ctx, member) ::= "<ctx>.<member>"
SubContextLocal(ctx, subctx, local) ::= "<ctx>.<subctx>.<local>"
SubContextMember(ctx, subctx, member) ::= "<ctx>.<subctx>.<member>"
Copy link
Member

@KvanTTT KvanTTT Dec 19, 2022

Choose a reason for hiding this comment

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

The changes related to test templates also should be extracted into another pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See previous comment

Copy link
Member

Choose a reason for hiding this comment

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

@ericvergnaud It would be nice to see the upgrade to the test specificity in a separate PR. Want me to pull those changes out into a PR we merge before your TypeScript target PR?

@parrt
Copy link
Member

parrt commented Dec 19, 2022

@parrt looks good to me, all tests pass for all targets on latest dev

yay! I would like to see the test updates in a separate PR. happy to do that for you if you want. Then we can merge both.

thanks for such a huge contribution!

@ericvergnaud
Copy link
Contributor Author

ericvergnaud commented Dec 19, 2022 via email

@KvanTTT
Copy link
Member

KvanTTT commented Dec 19, 2022

all the rest except maybe StructDecl, which I’m noticing now, there could be an incorrect merge with StructDecl, I’m 100% positive that I haven’t touched that stuff, so not sure why it shows up in the merge… also not sure how the go tests pass if that merge is incorrect

I highly recommend using interactive rebase in IntelliJ IDEA to filter out unrelated or incorrect changes and to prepare clear history.

@parrt
Copy link
Member

parrt commented Dec 19, 2022

Acknowledged @ericvergnaud @KvanTTT. I'll see if intellij can help me tease this apart.

tool/pom.xml Outdated
@@ -24,7 +24,7 @@
<dependency>
<groupId>org.antlr</groupId>
<artifactId>antlr4-runtime</artifactId>
<version>${project.version}</version>
<version>4.11.1</version>
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to use the project.version not 4.11.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

certainly, I don't remember making this change but it wasn't PR'd intentionnally

Copy link
Member

Choose a reason for hiding this comment

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

no doubt I did something dumb like that...

Still I wonder why the rebase or merge from dev into your branch...i'm noticing a few things. ALMOST done with new PR for your test updates.

@parrt
Copy link
Member

parrt commented Dec 19, 2022

all the rest except maybe StructDecl, which I’m noticing now, there could be an incorrect merge with StructDecl, I’m 100% positive that I haven’t touched that stuff, so not sure why it shows up in the merge… also not sure how the go tests pass if that merge is incorrect

yeah, weird. maybe it needed a rebase or something if you branched from dev earlier than these changes? Hmm...i'll have to be careful pulling this apart. I think looking at all changes and pulling those out manually is easier than merging interactively with ~180 commits haha.

@parrt
Copy link
Member

parrt commented Dec 19, 2022

@ericvergnaud yeah, it looks like you did a refactor/cleanup that whacked a previous changes in "make it easy to break" 974606e

Hmm...i wonder if i can do a rebase to make that merge properly....i wonder why a conflict didn't appear?

parrt added a commit to parrt/antlr4 that referenced this pull request Dec 20, 2022
…rebase after we merge this into dev. All tests pass locally (didn't check python2 actually but python3 works).

Signed-off-by: Terence Parr <[email protected]>
parrt added a commit to parrt/antlr4 that referenced this pull request Dec 20, 2022
…rebase after we merge this into dev. All tests pass locally (didn't check python2 actually but python3 works).

Signed-off-by: Terence Parr <[email protected]>
@parrt
Copy link
Member

parrt commented Dec 20, 2022

Ok, @ericvergnaud @KvanTTT I have two PRs that should be clean:

Nervous something is borked up here. Some merge conflicts got overridden. I think we should VERY carefully rebuild a PR for the TS target. Maybe get a clean branch from head of dev. Then copy all changed files from previous PR branch on top, which SHOULD highlight merge conflicts and be fresher :)

@ericvergnaud
Copy link
Contributor Author

ericvergnaud commented Dec 20, 2022 via email

# Conflicts:
#	runtime-testsuite/test/org/antlr/v4/test/runtime/FileUtils.java
#	tool/src/org/antlr/v4/codegen/model/ListenerFile.java
@ericvergnaud
Copy link
Contributor Author

@parrt thanks for the intermediate PRs. I've cleaned up the suspicious changes, all changes left are valid. Xing fingers that the build succeeds. If it does you can squash and merge.

@parrt
Copy link
Member

parrt commented Dec 20, 2022

If it does you can squash and merge.

acknowledged.

@ericvergnaud
Copy link
Contributor Author

@parrt All green! :-)

@parrt
Copy link
Member

parrt commented Dec 21, 2022

I'll reset 4.11.2 -> 4.12.0 in milestones. thanks, @ericvergnaud great job!

@parrt parrt merged commit 2c75e64 into antlr:dev Dec 21, 2022
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.