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

!js-string implementation of caml_string_concat fix #1684

Closed
wants to merge 1 commit into from

Conversation

rickyvetter
Copy link
Contributor

If you write some code like (note this is not an ascii x!):

str ^ " ×"

And have compiled your code with --disable use-js-string, then the concat will not work correctly. I've changed to an implementation that seems correct, but likely has quite a few extra conversions. I'm not sure exactly what the original function was doing, but it felt complicated and so I was a bit nervous about fixing in a lower-level way.

I'd like to have tests in place but I'm not sure what the infrastructure looks like for testing with these flags?

This probably does a lot of unecessary conversions, but it seems obviously correct and so a reasonable starting point.
@hhugo
Copy link
Member

hhugo commented Sep 26, 2024

then the concat will not work correctly.

What does that mean ? What's the behavior you're seeing ?

I'd like to have tests in place but I'm not sure what the infrastructure looks like for testing with these flags?

There are some test in compiler/tests-compiler/test_string.ml. They call Util.compile_and_parse but you might want to use Util.compile_and_run.

@hhugo
Copy link
Member

hhugo commented Sep 26, 2024

Your implementation should just be equivalent to the existing one. If you inline a few function call. You get;

  s1.t & 6 && caml_convert_string_to_bytes(s1);
  var a = s1.c;
  s2.t & 6 && caml_convert_string_to_bytes(s2);
  var b = s2.c;
  var x = a + b;
  return new MlBytes(0, x, x.lenght);

while we currently have

  s1.t & 6 && caml_convert_string_to_bytes(s1);
  s2.t & 6 && caml_convert_string_to_bytes(s2);
  return new MlBytes(s1.t, s1.c + s2.c, s1.l + s2.l);

Also I wasn't able to reproduce your issue.

@rickyvetter
Copy link
Contributor Author

Sorry for the noise. You're right. There's a more complicated issue that isn't with this primitive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants