gfngfn / Sesterl

An ML-like statically-typed Erlang
153 stars 5 forks source link

Fix non-ascii string/binary literals; Add multi-line binary literals #22

Closed michallepicki closed 3 years ago

michallepicki commented 3 years ago

Also: require (and ignore) one newline at the start of a string block edit: This change is to have first line aligned horizontally with other lines when you don't want a newline at the start.

michallepicki commented 3 years ago

Hmm I think this may not be handling UTF-8 characters correctly. But possibly we already had this problem for erlang FFI snippets

edit: OR they are escaped correctly but I need to pass some flags for erlang printing / writing to files.

michallepicki commented 3 years ago

I think the way to go would be to pass the binary to the erlang source mostly as is from the .sest source file, so remove the String.escaped from here when using the "" syntax and add /utf8 modifier in printed erlang binary literal. So the "" syntax would accept all escape sequences that the erlang <<""/utf8>> literal accepts.

For the new multiline ` syntax, newlines need to be handled additionally I think, and I would propose this new syntax to keep user's input "as is" so escape all escape sequences they include. Or just dump it in the erlang file as integers (less readable but maybe easier). What do you think?

michallepicki commented 3 years ago

It seems that this change is also causing wrong line numbers to be reported in the reported errors when e.g. a type error happens after such string block binary, this should probably be fixed before merging.

edit: This was fixed.

michallepicki commented 3 years ago

@gfngfn Please let me know if this would be a welcome addition and I can work on this more.

gfngfn commented 3 years ago

Oops, I’m sorry for the late response. The addition you are working on is largely welcomed. Thank you!

michallepicki commented 3 years ago

@gfngfn So this is semantically more-or-less done, how I see it:

I left some TODOs to think about and tests/documentation are missing, but I would appreciate your thoughts about this design.

michallepicki commented 3 years ago

I think this is ready for review/merge now :)

gfngfn commented 3 years ago

Thank you for looking into the implementation. I would like to review your modification.

michallepicki commented 3 years ago

@gfngfn How does it look after my updates from a week ago?

gfngfn commented 3 years ago

Oh, I’m so sorry for the late response and for not having given any suggestions… The updated change looks very nice to me!

gfngfn commented 3 years ago

The change made by this PR looks excellent and correct according to the added tests. I would then like to merge this PR. Thank you so much for the development!