Conversation
nojaf
left a comment
nojaf
left a comment
There was a problem hiding this comment.
I'm not sure if this is the way to go.
I'd prefer a solution where everything happens in one pass.
| // Fast path: check if string needs encoding at all | ||
| let needsEncoding = | ||
| text | ||
| |> Seq.exists (fun c -> |
There was a problem hiding this comment.
This scans the string to check if encoding is needed, but you traverse it again when you encoding.
Can we do this in one pass?
I'm a little afraid this code is in a hot path.
There was a problem hiding this comment.
The question I had is that, expecting this is a minor use-case (but AI does more and more documentation in the future) is it worth of checking every character ever that goes through formatting? Will it slowdown the process? So could we do somehow like a high-level check, if the replacement-run is relevant at all?
There was a problem hiding this comment.
checking every character ever that goes through Fantomas?
What do you mean here?
There was a problem hiding this comment.
sorry, I meant F# Formatting of course not Fantomas. I was thinkin only in context of html generation: It is called in basically every .NET tool build-scripts, we want it to be fast if possible.
| then | ||
| let fullCodePoint = Char.ConvertToUtf32(c, text.[i + 1]) | ||
| // Encode all characters outside BMP (>= 0x10000) as they're typically emojis | ||
| sb.Append(sprintf "&#%d;" fullCodePoint) |> ignore |
There was a problem hiding this comment.
Isn't sprintf allocating here and beats the point of the StringBuilder a bit?
There was a problem hiding this comment.
I'm not sure of that. I've tried to check the sprintf with https://sharplab.io/ before, but I wasn't convinced.
2 participants
This fixes #964