Added examples for Memory<T> guidance #343
Conversation
There was a problem hiding this comment.
These are good samples @rpetrusha
I had a few suggestions, all toward the goal of showing better production-code style even though it is not related to the concepts being demonstrated here.
I'm not sure the best solution for the Task.Run call using the ReadOnlyMemory. If possible, that should be async. If not, the supporting text should explain that limitation in detail.
| Console.WriteLine($"You entered a number less than {Int32.MinValue:N0} or greater than {Int32.MaxValue:N0}."); | ||
| } | ||
| finally { | ||
| owner.Dispose(); |
There was a problem hiding this comment.
minor nit: I'd prefer owner?.Dispose(); to protect against the statement in line 8 throwing.
| { | ||
| // <Snippet1> | ||
| // An acceptable implementation. | ||
| static void Log(ReadOnlyMemory<char> message) |
There was a problem hiding this comment.
I'd prefer seeing this method return the Task so that it can be awaited.
Unless, what you're showing is how ReadOnlyMemory is limited in it use because it is a ref struct. I didn't compile this code, but I think this use is allowed, but the caller may be limited.
| return; | ||
|
|
||
| int numCharsWritten = ToBuffer(value, span); | ||
| Log(memory.Slice(0, numCharsWritten)); |
There was a problem hiding this comment.
If Log returns a Task, this can be awaited (and main can be an async main method).
| { | ||
| // <Snippet1> | ||
| // An acceptable implementation. | ||
| static void Log(ReadOnlyMemory<char> message) |
There was a problem hiding this comment.
Same comment on async as the previous samples.
There was a problem hiding this comment.
That won't work, since spans aren't allowed in async methods.
There was a problem hiding this comment.
@rpetrusha @BillWagner does the code use this ReadOnlyMemory struct? Then it's not ref struct, but usual readonly struct. Then, I expect it can be used in async methods. Or I'm wrong here?
There was a problem hiding this comment.
@pkulikov You are right. I read Ron's related PR in dotnet/docs and yes, Memory and ReadonlyMemory are not ref structs.
There was a problem hiding this comment.
Oh, I see: though the Log might be async, it, anyway, cannot be awaited in the Main as Main contains spans and cannot be async. And not awaiting async methods might be not a good example.
There was a problem hiding this comment.
I apologize for commenting on a merged PR. But my comments are related to dotnet/docs#8055, which is still open and I'm not sure what would be the right fix for some of them, which is why I didn't open a PR instead.
|
|
||
| var memory = owner.Memory; | ||
| WriteInt32ToBuffer(value, memory); | ||
| DisplayBufferToConsole(memory.Slice(0, value.ToString().Length)); |
There was a problem hiding this comment.
If you use value.ToString(), then that defeats the purpose of using Memory, because it allocates. I think a better option would be if WriteInt32ToBuffer returned how many bytes it wrote.
| { | ||
| static void Main() | ||
| { | ||
| using (IMemoryOwner<char> owner = MemoryPool<char>.Shared.Rent()) |
There was a problem hiding this comment.
What are the guarantees about the size of the returned buffer when you don't specify minBufferSize, like here? The documentation for MemoryPool<T> is missing, so that doesn't help.
WriteInt32ToBuffer relies on the fact that it's at least ~11 bytes, which is almost certainly nowhere near the limit. But most Memory-related code will have to deal with the possibility of a too small buffer somehow, so maybe it's worth handling here?
|
|
||
| static void WriteInt32ToBuffer(int value, Memory<char> buffer) | ||
| { | ||
| var strValue = value.ToString(); |
There was a problem hiding this comment.
Shouldn't this use int.TryFormat to avoid the string allocation?
| } | ||
| catch (OverflowException) { | ||
| Console.WriteLine($"You entered a number less than {Int32.MinValue:N0} or greater than {Int32.MaxValue:N0}."); | ||
| } |
There was a problem hiding this comment.
How are these catches useful here? I would understand them if they were related to Memory, but they're not.
| } | ||
|
|
||
| static void DisplayBufferToConsole(Memory<char> buffer) => | ||
| Console.WriteLine($"Contents of the buffer: '{buffer}'"); |
There was a problem hiding this comment.
Similarly, I think it would be better if this code didn't allocate, but I can't think of a reasonable way to achieve that here.
| @@ -0,0 +1,55 @@ | |||
| // <Snippet1> | |||
There was a problem hiding this comment.
Why does this file have two separate <Snippet1> tags?
| { | ||
| // Run in the background so that we don't block the main thread while performing IO. | ||
| Task.Run(() => { | ||
| string defensiveCopy = message.ToString(); |
There was a problem hiding this comment.
I think this defensive copy is too late. The Task could start executing long after Log returns, at which point message could be already in a different state or destroyed.
The defensive copy should happen before the Task.Run and the lambda should only access defensiveCopy, not message. (At which point I think it would be possible to even use Span instead of Memory.)
Added examples for Memory<T> guidance
Related to dotnet/docs#4823