Simplify & optimize Replace #238
Conversation
| { | ||
| Span<char> tempBuffer = stackalloc char[24]; | ||
| if (spanFormattable.TryFormat(tempBuffer, out var written, default, null)) | ||
| Span<char> tempBuffer = stackalloc char[128]; |
There was a problem hiding this comment.
Is there any ISpanFormattable where the string representation is that long? (Given that you cannot pass in a IFormatProvider).
There was a problem hiding this comment.
I don't know but users can make their own ISpanFormattable types, so the fallback to ToString is required. However, 128 chars might be too much, it's up to you if you'd like to change it (I just went with the number used elsewhere in the repository)
There was a problem hiding this comment.
That is a good point. The 24 was used as even decimal and double will not take more space than that when no custom provider is used.
I don’t see problem per-se with 128 bytes. I am no embedded guy to judge it might be too much. We could also settle on 64 byte or 32 and raise it once we have complaints :)
There was a problem hiding this comment.
Yeah, it should make barely any difference performance-wise since it's only 256 bytes and very short-lived. I think 64 chars should work fine as well.
Also, should IFormatProvider? be added as an argument?
There was a problem hiding this comment.
Interesting questio. If so I wouldn’t make it optional and provably would ask the caller about the size.
So there would an additional overload. I don’t think much would speak against is, as we only have to pass it through
There was a problem hiding this comment.
I'm thinking that asking the caller about the size would be redundant, because it's not recommended to stackalloc more than 256 bytes anyway. It's up to you if you want a new overload or an optional argument though. Bare in mind that double.TryFormat (among others) use an optional format provider argument (likely to implement the interface).
There was a problem hiding this comment.
That is true - we have to check if it is bigger than a certain size and have to resort to an ArrayPool if so.
Unfortunately the Append method doesn’t do that right now and would crash the application :) so therefore good catch.
If we go down that route, also the append method should get the IFormatProvider bit.
Yeah probably it is okay to stick to that pattern. It isn’t more explicit just because you have to set it in another overload. I am not sure if we can assume that with a custom formatprovider the written span will always be less than a certain size.
|
I also added |
Really? I thought it might resort to a boxed invocation. We can also do this for v3 if that isn’t the case. For now, because of semantic versioning, I would like to keep it as is. The scoped though is absolutely fine :) |
|
|
I made some minor clarity changes. It's ready to be merged unless you have more concerns. |
2 participants
There are two reasonable approaches to replacing a value with another value of a different length:
Previously, the replace operation did a combination of the two which seemed over-the-top. I changed it to only use the second approach, which is far simpler and gives better performance!
BEFORE:
AFTER:
Note:
Two of the tests had to be changed because, in my opinion, you shouldn't be allowed to give out-of-bounds parameters just because another parameter is empty and short-circuits.