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

Improve replacements for StringBuilder. #1500

Closed
Alxandr opened this issue Jul 22, 2018 · 15 comments
Closed

Improve replacements for StringBuilder. #1500

Alxandr opened this issue Jul 22, 2018 · 15 comments

Comments

@Alxandr
Copy link
Contributor

Alxandr commented Jul 22, 2018

I currently have a shim StringBuilder in my code so as to be able to use APIs that needs stringbuilders. This should probably be part of Fabe.Core (with replacements in place). In JS, it can be implemented as a simple ref<string> or similar.

@ncave
Copy link
Collaborator

ncave commented Jul 22, 2018

@Alxandr It's already part of Fable.Core, just use it.

@Alxandr
Copy link
Contributor Author

Alxandr commented Jul 22, 2018

@ncave That's weird, cause I've been getting exceptions that no replacement was found for StringBuilder.Append (etc.)

@ncave
Copy link
Collaborator

ncave commented Jul 22, 2018

It was added recently, try it.

@Alxandr
Copy link
Contributor Author

Alxandr commented Jul 22, 2018

You're right. I still get errors though 😛:

[!] Error: 'StringBuilder$$Append$$244C7CD6' is not exported by .fable\fable-core.2.0.0-alpha-031\System.Text.js

@Alxandr Alxandr changed the title Provide replacements for StringBuilder. Improve replacements for StringBuilder. Jul 22, 2018
@ncave
Copy link
Collaborator

ncave commented Jul 22, 2018

@Alxandr The current SB implementation is a bit simplistic, probably one of the overloads is missing, as the overload mangling is now done based on argument types. Post a bit of F# code so we can see which one.

@Alxandr
Copy link
Contributor Author

Alxandr commented Jul 22, 2018

I currently use the following implementation as a shim (be adviced that Remove and ToString with indexes has not been tested):

  type StringBuilder (s: string) =
    let mutable s = s

    new () = StringBuilder ("")

    member __.Length = s.Length

    member sb.Append (s': string) =
      s <- s + s'
      sb

    member inline sb.Append (c: char) = sb.Append (string c)

    member inline sb.Append (num: ^n) = sb.Append (sprintf "%d" num)

    member inline sb.Append (o: obj) = sb.Append (string o)

    member inline sb.AppendLine () = sb.Append System.Environment.NewLine

    member inline sb.AppendLine (s: string) =
      (sb.Append (s)).AppendLine ()

    member sb.Remove (startIndex: int, length: int) =
      if startIndex + length >= s.Length
      then s <- s.Substring (0, startIndex)
      else s <- s.Substring (0, startIndex) + s.Substring (startIndex + length)
      sb

    member inline __.ToString (startIndex: int, length: int) =
      s.Substring (startIndex, length)

    override __.ToString() = s

Not entirely sure which are missing, but that's at least the minimal implementation for my sake (wrt which methods are available).

@ncave
Copy link
Collaborator

ncave commented Jul 22, 2018

@Alxandr Eventually all StringBuilder.Append overloads will have to be added. If you need to get rid of your shim sooner, you can submit a PR, at least for the trivial ones, it should work now that @alfonsogarciacaro fixed the overload mangling.

@Alxandr
Copy link
Contributor Author

Alxandr commented Jul 22, 2018

Yeah, no, the shim works, so this isn't a rush.

@Alxandr
Copy link
Contributor Author

Alxandr commented Jul 23, 2018

I can work on this issue now that the test issues fixes has been merged 🙂

@alfonsogarciacaro
Copy link
Member

It'd be nice to have a proper implementation that actually prevents many string allocations, using a typed char array (another reason to merge #1473.

BTW, now that we can use F# for fable-core, it'd be also nice to translate the System.Decimal source to F# so we can actually keep accuracy. Just saying ;)

@Alxandr
Copy link
Contributor Author

Alxandr commented Jul 23, 2018

I think things like System.Decimal is probably a better fit for webassembly (like long.js did). As for string builder, as far as I've seen from tests, JS engines are really well optimized at appending strings together (probably because that's the only way you've been able to build strings), that I don't think the added complexity is worth it. At least not unless someone does a propper meassure to see that it is.

@Alxandr
Copy link
Contributor Author

Alxandr commented Jul 23, 2018

Speaking of long.js, should it maybe be updated from upstream? Wouldn't it be better if it could be imported as a file untouched, and simply write the fable shims needed on top?

@alfonsogarciacaro
Copy link
Member

That's what we did for Fable 1, but long.js attaches everything to the prototype so any import would bring the whole file. That's why I changed the functions to get better advantage of tree shaking. I know it makes updating the file more complicated but...

@Alxandr
Copy link
Contributor Author

Alxandr commented Jul 23, 2018

Ah. You're right. I didn't notice.

@alfonsogarciacaro
Copy link
Member

Let's close the issue as we already have PR #1505 to track this.

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

No branches or pull requests

3 participants