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

Remove forward ref in the lexer #168

Merged

Conversation

hhugo
Copy link
Contributor

@hhugo hhugo commented Jul 20, 2023

The forward ref make jsoo deadcode elimination ineffective.

Fix #84 to some degree.

The forward ref make jsoo deadcode elimination ineffective
@hhugo
Copy link
Contributor Author

hhugo commented Jul 20, 2023

@Leonidas-from-XIV, feel free to take over this PR.

@hhugo hhugo mentioned this pull request Aug 2, 2023
@Leonidas-from-XIV Leonidas-from-XIV added the no changelog Not a user visible change, does not require changelog entry label Aug 2, 2023
Copy link
Member

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me and honestly a bit nicer than the optjunk32 business.

I think this area of Yojson is under-tested since we have any tests on parsing errors.

@hhugo
Copy link
Contributor Author

hhugo commented Oct 9, 2023

@Leonidas-from-XIV, can this be merged ?

@Leonidas-from-XIV
Copy link
Member

@hhugo If you consider this PR finished (to me it seems so), it looks fine to me and ok to merge.

@hhugo
Copy link
Contributor Author

hhugo commented Oct 9, 2023

This PR is ready from my perspective

@Leonidas-from-XIV Leonidas-from-XIV merged commit 7f06724 into ocaml-community:master Oct 9, 2023
1 check passed
@Leonidas-from-XIV
Copy link
Member

Thanks for the contribution! Might be worth cutting a patch release to help JSOO users.

Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Oct 10, 2023
CHANGES:

*2023-10-10*

### Changed

- Make `Basic`, `Safe` & `Raw` seperate compilation units that get exposed by
  the main module as suggested by @hhugo to enable JSOO to discard unused
  modules. No API changes should be observable. (ocaml-community/yojson#84, ocaml-community/yojson#167 @Leonidas-from-XIV)
- Removed forward refs in the parser to make dead-code elimination in JSOO
  better (ocaml-community/yojson#168, @hhugo)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Oct 10, 2023
CHANGES:

*2023-10-10*

- Make `Basic`, `Safe` & `Raw` seperate compilation units that get exposed by
  the main module as suggested by @hhugo to enable JSOO to discard unused
  modules. No API changes should be observable. (ocaml-community/yojson#84, ocaml-community/yojson#167 @Leonidas-from-XIV)
- Removed forward refs in the parser to make dead-code elimination in JSOO
  better (ocaml-community/yojson#168, @hhugo)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Oct 10, 2023
CHANGES:

*2023-10-10*

- Make `Basic`, `Safe` & `Raw` seperate compilation units that get exposed by
  the main module as suggested by @hhugo to enable JSOO to discard unused
  modules. No API changes should be observable. (ocaml-community/yojson#84, ocaml-community/yojson#167 @Leonidas-from-XIV)
- Removed forward refs in the parser to make dead-code elimination in JSOO
  better (ocaml-community/yojson#168, @hhugo)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

*2023-10-10*

- Make `Basic`, `Safe` & `Raw` seperate compilation units that get exposed by
  the main module as suggested by @hhugo to enable JSOO to discard unused
  modules. No API changes should be observable. (ocaml-community/yojson#84, ocaml-community/yojson#167 @Leonidas-from-XIV)
- Removed forward refs in the parser to make dead-code elimination in JSOO
  better (ocaml-community/yojson#168, @hhugo)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Not a user visible change, does not require changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Large output size on jsoo
2 participants