Jump to conversation
Unresolved conversations (0)
Nice work!

Nice work!

All of your conversations have been resolved.

Resolved conversations (16)
@am11 am11 Jul 12, 2021
@stephentoub, just curious, has the default of string interpolation changed from current culture to InvariantCulture?
...n/src/System/CodeDom/CodeTypeReference.cs
stephentoub
@333fred 333fred Jul 9, 2021
Isn't this a behavior change? Interpolated strings use current culture, not invariant.
...on/src/System/Drawing/Printing/Margins.cs
stephentoub
@333fred 333fred Jul 9, 2021
Oops.
Outdated
Directory.Build.props
stephentoub
@bartonjs bartonjs Jun 30, 2021
I'd probably remove the comment, too.
Outdated
...stem.Private.CoreLib/src/System/Random.cs
@bartonjs bartonjs Jun 30, 2021
Since AppendFormattedSlow is only called when _hasCustomFormatter was true (already returned), value was null (already returned) or TryCopyTo failed, we can be pretty certain the body of this if is unreachable, right? I'd probably put a Debug.Fail in it to justify its uncovered state to someone looking at ccov.
...te.CoreLib/src/System/MemoryExtensions.cs
stephentoub
@bartonjs bartonjs Jun 30, 2021
```suggestion await connection.WriteStringAsync($"{bytesToSend:X}{lineEnding}"); ```
Outdated
...ests/System/Net/Http/HttpProtocolTests.cs
stephentoub
@GrabYourPitchforks GrabYourPitchforks Jun 10, 2021
Nit: Since `ISpanFormattable` is now public, this could expose uninitialized memory to the caller, as `StringBuilder` uses `GC.AllocateUninitializedArray` under the covers. In the worst case, this could allow me to write a buggy `ISpanFormattable` implementation as follows. ```cs public class MyClass : ISpanFormattable { public bool TryFormat(Span<char> destination, out int charsWritten, ...) { charsWritten = destination.Length; return true; } } ``` If I then pass an instance of that type into the new `StringBuilder.Append` overloads, the resulting `StringBuilder` will contain data populated from uninitialized memory, even though the caller (me) never wrote unsafe code or used any unsafe-equivalent API. Now, I don't think this is a problem in practice, as nobody would ever write an `ISpanFormattable` like that. But it's an example of where we're starting to play a little fast and loose with type \& memory safety, and we may need to start considering how this affects more common APIs like `StringBuilder` (and potentially `MemoryStream`, if we decide to make these same changes there). Are these patterns deserving of mention on MSDN? Is it even worth spending any time on this at all? Maybe in the extreme it's not much different than what we already have in the `Stream.Read[Async]` base implementations, which use the array pool, and I'm being overly paranoid.
....CoreLib/src/System/Text/StringBuilder.cs
stephentoub
@bartonjs bartonjs Apr 22, 2021
Do we already have a test that writes more than initialBuffer.Length?
Outdated
...ystem.Runtime/tests/System/StringTests.cs
stephentoub
@bartonjs bartonjs Apr 22, 2021
I don't think I saw a test that hits this. Either from a bad implementation reporting a negative number, or too big of one.
....CoreLib/src/System/Text/StringBuilder.cs
stephentoub
@bartonjs bartonjs Apr 21, 2021
```C# string[] names = { "param1", "param2" }; var attr3 = new InterpolatedBuilderArgumentAttribute(names); Assert.Same(names, attr3.Arguments); ``` ?
Outdated
...ntime/CompilerServices/AttributesTests.cs
stephentoub
@bartonjs bartonjs Apr 21, 2021
As the compiler pattern wouldn't this be ```suggestion AppendFormatted(builder.ToStringAndClear(), alignment); ``` ? I'm surprised we even have an allocating Text property.
Outdated
....CoreLib/src/System/Text/StringBuilder.cs
stephentoub
@bartonjs bartonjs Apr 21, 2021
I assume `format` is an ignored argument because a) The pattern requires it. b) `string.Format("{0:X}", "hi");` doesn't complain that format specifiers make no sense in context, so we won't here?
....CoreLib/src/System/Text/StringBuilder.cs
stephentoub
@bartonjs bartonjs Apr 21, 2021
Would ```C# else if (alignment < 0) { int start = _builder.Length; AppendFormatted(value, format); int end = _builder.Length; int pad = alignment - end - start; // Whatever to do to Append `pad` spaces here } ``` be more space-and-time efficient? The left pad shouldn't need to rent, and either way it avoids the double copy. I'm perfectly willing to believe "no, it's not more efficient", just asking.
....CoreLib/src/System/Text/StringBuilder.cs
stephentoub
@bartonjs bartonjs Apr 21, 2021
In `StringBuilder` you were defensive against bad TryFormat implementations reporting they wrote out of bounds, but here you weren't.
...te.CoreLib/src/System/MemoryExtensions.cs
stephentoub
@bartonjs bartonjs Apr 21, 2021
All of the references to "holes" probably want to be "placeholders", or something from a language spec.
Outdated
....CoreLib/src/System/Text/StringBuilder.cs
stephentoub
@bartonjs bartonjs Apr 21, 2021
I get it. But, wow. That looks broken :smile:.
Outdated
....CoreLib/src/System/Text/StringBuilder.cs
stephentoub