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

[Dart] Support Dart multiple platforms. #3250

Merged
merged 12 commits into from Aug 23, 2021
Merged

[Dart] Support Dart multiple platforms. #3250

merged 12 commits into from Aug 23, 2021

Conversation

ghost
Copy link

@ghost ghost commented Aug 8, 2021

fixes #3190

Support Dart multiple platforms.

  • Uses conditional import / export on dart:io.
  • Defines 32bit version bit set operations as JS version does not operate on 64bit integers.
  • Migrates from dart2native to dart compile exe.
  • Runs dartfmt on Dart runtime.

Yi-Hong Lin added 7 commits August 7, 2021 14:37
*  `dart compile exe` is preferred since Dart v2.10 .
* Reorganize dart:io usages and support 'io', 'html'.
  * 'ui' and 'isolate' TBD.
* Rewrite bit operations with 32-bit version because Dart2JS does not support 64-bit integers.
* Reorganize dart:io usages and support 'io', 'html'.
  * 'ui' and 'isolate' TBD.
* Rewrite bit operations with 32-bit version because Dart2JS does not support 64-bit integers.
@ghost ghost closed this Aug 8, 2021
@ghost ghost reopened this Aug 8, 2021
Copy link
Member

@lingyv-li lingyv-li left a comment

Choose a reason for hiding this comment

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

Overall it looks pretty good! 👍
You also need to remove this import, potentially with a hack:

This is how I would do it:
lingyv-li@2843096#diff-6e09474b1421fd5d94035b5b34836896dbf8534f6e7b65cb860590c36b314aabR2

Also since you've removed the locateDart2native already, can you please also remove locatePub?

runtime/Dart/lib/src/error/error.dart Outdated Show resolved Hide resolved
Yi-Hong Lin added 2 commits August 9, 2021 13:41
* Reorganize platform dependent files.
* Replace `pub` with `dart pub` in tests.
* Update runtime template to be platform dependent.
@ghost ghost requested a review from lingyv-li August 9, 2021 20:59
Copy link
Member

@lingyv-li lingyv-li left a comment

Choose a reason for hiding this comment

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

Also, can you show a working demo of it on web? Would be good to show it working against e.g. Java 9 grammar https://github.com/antlr/grammars-v4/tree/master/java/java9

runtime/Dart/lib/src/util/platform_html.dart Show resolved Hide resolved
@ghost ghost marked this pull request as draft August 10, 2021 17:09
@ghost ghost marked this pull request as ready for review August 11, 2021 00:39
@ghost
Copy link
Author

ghost commented Aug 11, 2021

@lingyv-li I did not know how to demo in Github (more specifically, presenting java9 grammar in web). I ended up creating two demos, one for web version and command line version. Please take a look.

@ghost ghost requested a review from lingyv-li August 11, 2021 00:45
Copy link
Member

@lingyv-li lingyv-li left a comment

Choose a reason for hiding this comment

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

Hey @ansiemens sorry it wasn't clear, and the Java grammar is a little tricky to work with. Don't worry I just tried and built a demo app from your branch for Java, it works great!

Screen.Recording.2021-08-11.at.11.36.51.pm.mov

https://github.com/lingyv-li/java9parser/tree/cc6318a0e3072ff2651fa704f2ff4cf331519ffc

Please remove the example folder that you've just added.

Comment on lines 6 to 8
## 4.9.3-SNAPSHOT

* Support multi-platform.
Copy link
Member

Choose a reason for hiding this comment

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

This file needs to follow semantic versioning. Change it to 4.9.3 so it will be used when publishing that version.

Also it's better described as "Support web platform" since it already support other native platforms before this version.

Suggested change
## 4.9.3-SNAPSHOT
* Support multi-platform.
## 4.9.3
* Support web platform.

@ghost
Copy link
Author

ghost commented Aug 11, 2021

Cool! Updated the PR. PTAL.

@ghost ghost requested a review from lingyv-li August 11, 2021 15:07
Copy link
Member

@lingyv-li lingyv-li left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @ansiemens
@ericvergnaud can you pls check & merge this PR

@ghost
Copy link
Author

ghost commented Aug 12, 2021

cc @parrt

@parrt parrt merged commit e07593b into antlr:master Aug 23, 2021
@parrt parrt added this to the 4.9.3 milestone Oct 11, 2021
@cesarParra
Copy link

This seems to not be working anymore when using the latest version of ANTLR4 (4.11.1).

Here's a portion of a diff when running it through 4.9.3 (left) vs. 4.11.1 (right).

image

@KvanTTT
Copy link
Member

KvanTTT commented Oct 13, 2022

The change doesn't relate to the issue. Could you please elaborate on encountered errors?

@parrt
Copy link
Member

parrt commented Oct 13, 2022

@cesarParra perhaps you could start a new issue with a sample grammar and input that causes the problem.

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.

Flutter/Dart web support
5 participants