- Notifications
You must be signed in to change notification settings - Fork471
Move Option stdlib optimizations into typed pipeline#7921
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:options-stdlib-opt
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| returncategory.name; | ||
| let__res_option_opt; | ||
| if(incidentId!==undefined){ | ||
| letincidentId$1=Primitive_option.valFromOption(incidentId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Output looks worse than the Parsetree version.
ThesePrimitive_option.valFromOption should not be needed.
| functiontestPrimitive(){ | ||
| console.log(42); | ||
| letvalue=42; | ||
| console.log(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why did we lose the optimization here?
As we are doing the transformation on the typed level now, shouldn't we add some type checking tests, too? |
| ```rescript | ||
| Js.String.charCodeAt(0, `😺`) == 0xd83d->Belt.Int.toFloat | ||
| Js.String.codePointAt(0, `😺`) == Some(0x1f63a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you removing this because it's not really an example forcharCodeAt or because the test failed? (Actually this should be correct / the test should work.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Don't know it failed before already. Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You may need to remove tests/docstring_tests/generated_mocha_test.res and re-run the tests.
9202879 tofd1a69dCompare| letcategory=categoryId!==undefined ?Belt_MapString.get(categories,categoryId) :undefined; | ||
| if(category!==undefined){ | ||
| returncategory.name; | ||
| let__res_option_opt=incidentId!==undefined ?Belt_MapString.get(incidents,incidentId) :undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Now the output is the same except for worse variable naming. 🙂
Can we produce the identical output?
Uh oh!
There was an error while loading.Please reload this page.
f1692ff to4496782Compare97812a7 to5ac5fc9Compare4496782 to50dbc3cCompare# Conflicts:#packages/@rescript/runtime/Stdlib_DataView.resi
5ac5fc9 to5171806CompareRebased after the Docstring test fixes |
Summary
Testing