Skip to content

Compiler: cleanup usage of IString (now NativeString) vs String #984

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 13, 2021
Merged

Compiler: cleanup usage of IString (now NativeString) vs String #984

merged 5 commits into from
Dec 13, 2021

Conversation

hhugo
Copy link
Member

@hhugo hhugo commented Apr 9, 2020 •
edited
Loading

IString was introduced in #22 as a way to represent JavaScript strings and allow for more optimizations.

For example:

| Let(x, Prim (Extern "caml_js_from_string", Pc (String x))) when String.is_ascii s ->
  Let(x, Constant (IString x))

Over time, this distinction got lost a bit and new code started to use String and IString inconsistently.

What this PR does:

  • rename IString into NativeString to be more descriptive.
  • go over the codebase and review all usage.

Note for reviewers:

  • NativeString represent a js string (Js.js_string Js.t)
  • String represent an ocaml string

@hhugo hhugo force-pushed the fix-istring branch 3 times, most recently from 492de4e to 06cef74 Compare April 14, 2020 09:57
@hhugo hhugo requested a review from TyOverby April 14, 2020 17:50
@hhugo
Copy link
Member Author

hhugo commented Apr 22, 2020

Should release this before #976

@hhugo hhugo added this to the 3.7 milestone Apr 24, 2020
@TyOverby
Copy link
Collaborator

@hhugo could you add a description to the pull request? What NativeString represents, and what the change accomplishes would be great

hhugo
hhugo commented Apr 25, 2020
View reviewed changes
@hhugo
Copy link
Member Author

hhugo commented Apr 25, 2020

@TyOverby, I've updated the description

@hhugo hhugo force-pushed the fix-istring branch 4 times, most recently from 5667021 to 583b21b Compare May 12, 2020 09:54
@hhugo hhugo requested a review from xclerc July 31, 2020 09:32
@hhugo
Copy link
Member Author

hhugo commented Jul 31, 2020

@xclerc, I've just rebased this feature and added you as a reviewer.

xclerc
xclerc reviewed Aug 3, 2020
View reviewed changes
@hhugo hhugo removed this from the 3.7 milestone Nov 12, 2020
@hhugo hhugo force-pushed the fix-istring branch 7 times, most recently from e33dce0 to a6d7b69 Compare November 20, 2021 23:44
@hhugo
Copy link
Member Author

hhugo commented Nov 20, 2021

@xclerc, I know it has been a long time. I worked on this PR a bit more and would appreciate another round of review if you have time.

@hhugo hhugo added this to the 3.12 milestone Nov 21, 2021
@hhugo hhugo mentioned this pull request Nov 21, 2021
Merged
5 tasks
@hhugo
Copy link
Member Author

hhugo commented Dec 7, 2021

@xclerc , ping.
Let me know if you don't have time to look at this or if you have any ETA for review ?

@hhugo hhugo requested a review from xclerc December 8, 2021 23:08
@xclerc
Copy link
Collaborator

xclerc commented Dec 9, 2021

Sorry, I missed your previous message.
I will have a look this week or early next week.

xclerc
xclerc approved these changes Dec 9, 2021
View reviewed changes
Copy link
Collaborator

@xclerc xclerc left a comment

Choose a reason for hiding this comment

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

The diff looks reasonable, but my knowledge
of the code base is too shallow to take this
approval seriously...

hhugo and others added 4 commits December 12, 2021 12:33
@hhugo
Compiler: cleanup String vs IString
3b12f05
@hhugo @xclerc
Update CHANGES.md
93c47de 
Co-authored-by: Xavier Clerc <xclerc@users.noreply.github.com>
@hhugo @xclerc
Update runtime/fs.js
3659ef1 
Co-authored-by: Xavier Clerc <xclerc@users.noreply.github.com>
@hhugo
CR
3224438
@hhugo
Copy link
Member Author

hhugo commented Dec 12, 2021

@xclerc, thanks for your time reviewing this.

@hhugo hhugo merged commit 1fc8301 into master Dec 13, 2021
@hhugo hhugo deleted the fix-istring branch December 13, 2021 11:16
@hhugo hhugo mentioned this pull request Jan 24, 2022
Merged
hhugo added a commit to hhugo/opam-repository that referenced this pull request Jan 24, 2022
@hhugo
[new release] js_of_ocaml, js_of_ocaml-tyxml, js_of_ocaml-toplevel, j…
5f6301d 
…s_of_ocaml-ppx_deriving_json, js_of_ocaml-ppx, js_of_ocaml-lwt and js_of_ocaml-compiler (4.0.0)

CHANGES:

## Features/Changes
* Compiler: add --target-env flag, for JS runtime specific compilation targets (ocsigen/js_of_ocaml#1160).
* Compiler: static evaluation of backend_type (ocsigen/js_of_ocaml#1166)
* Compiler: speedup emitting js files (ocsigen/js_of_ocaml#1174)
* Compiler: simplify (a | 0) >>> 0 into (a >>> 0) (ocsigen/js_of_ocaml#1177)
* Compiler: improve static evaluation of cond (ocsigen/js_of_ocaml#1178)
* Compiler: be more consistent dealing with js vs ocaml strings (ocsigen/js_of_ocaml#984)
* Compiler: Compiler: add BigInt to provided symbols (fix ocsigen/js_of_ocaml#1168) (ocsigen/js_of_ocaml#1191)
* Compiler: use globalThis, drop joo_global_object ocsigen/js_of_ocaml#1193
* Compiler: new -Werror flag to turn wanrings into errors (ocsigen/js_of_ocaml#1222)
* Compiler: make the inlining less agressive, reduce size, improve pref (ocsigen/js_of_ocaml#1220)
* Compiler: rename internal library js_of_ocaml-compiler.runtime to js_of_ocaml-compiler.runtime-files
* Lib: new runtime library to improve compatibility with Brr and gen_js_api
* Lib: add messageEvent to Dom_html (ocsigen/js_of_ocaml#1164)
* Lib: add PerformanceObserver API (ocsigen/js_of_ocaml#1164)
* Lib: add CSSStyleDeclaration.{setProperty, getPropertyValue, getPropertyPriority, removeProperty} (ocsigen/js_of_ocaml#1170)
* Lib: make window.{inner,outer}{Width,Height} non-optional
* Lib: introduce Js_of_ocaml.Js_error module, deprecate Js_of_ocaml.Js.Error exception.
* Lib: add deprecation warning for deprecated code
* PPX: json can now be derived for mutable records (ocsigen/js_of_ocaml#1184)
* Runtime: use crypto.getRandomValues when available (ocsigen/js_of_ocaml#1209)
* Misc: move js_of_ocaml-ocamlbuild out to its own repo
* Misc: add support for OCaml 4.14 (ocsigen/js_of_ocaml#1173)

## Bug fixes
* Compiler: fix sourcemap warning for empty cma (ocsigen/js_of_ocaml#1169)
* Compiler: Strengthen bound checks. (ocsigen/js_of_ocaml#1172)
* Compiler: fix `--wrap-with-fun` under node (ocsigen/js_of_ocaml#653, ocsigen/js_of_ocaml#1171)
* Compiler: fix parsing of annotaions in js stubs (ocsigen/js_of_ocaml#1212, fix ocsigen/js_of_ocaml#1213)
* Ppx: allow apostrophe in lident (fix ocsigen/js_of_ocaml#1183) (ocsigen/js_of_ocaml#1192)
* Runtime: fix float parsing in hexadecimal form
* Runtime: fix implementation of caml_js_instanceof
* Graphics: fix mouse_{x,y} (ocsigen/js_of_ocaml#1206)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xclerc xclerc xclerc approved these changes

@TyOverby TyOverby Awaiting requested review from TyOverby

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.12
Development

Successfully merging this pull request may close these issues.
3 participants
@hhugo @TyOverby @xclerc