Adjust PrintMembers to avoid boxing and avoid extra space #47095
Conversation
| F.WellKnownMethod(WellKnownMember.System_Text_StringBuilder__AppendObject), | ||
| F.Convert(F.SpecialType(SpecialType.System_Object), value)))); | ||
| Debug.Assert(value.Type is not null); | ||
| if (value.Type.IsValueType) |
There was a problem hiding this comment.
does this work for thigns like where T : struct type parameters? #Resolved
There was a problem hiding this comment.
it does. I'll add an explicit test
| // builder.Append(<name>); | ||
| // builder.Append(" = "); | ||
| // builder.Append((object)<value>); | ||
| // builder.Append(<value>.ToString()); OR builder.Append(<value>.ToString()); |
There was a problem hiding this comment.
.ToString() [](start = 77, length = 18)
(object)<value> #Closed
|
|
||
| // this.print(builder); | ||
| block.Add(F.ExpressionStatement(F.Call(F.This(), _printMethod, builderLocal))); | ||
| // if (this.print(builder)) builder.Append(" "); |
There was a problem hiding this comment.
// if (this.print(builder)) builder.Append(" "); [](start = 16, length = 48)
If PrintMembers() is only used internally, to implement ToString(), could this if (this.print(builder)) be removed and simply have PrintMembers() add a " " before each member? #ByDesign
There was a problem hiding this comment.
could this
if (this.print(builder))be removed and simply havePrintMembers()add a " " before each member?
Given that PrintMembers() or ToString() can be implemented explicitly, it seems either conditionally adding a trailing space here, or requiring a leading space before each member, will make one of the two methods somewhat awkward.
In reply to: 476183479 [](ancestors = 476183479)
There was a problem hiding this comment.
I feel like the simpler implementation, for both us and consumers, is to just require a leading space in front of all members. Then there's no conditionals or questions.
In reply to: 476188140 [](ancestors = 476188140,476183479)
There was a problem hiding this comment.
I think it's more regular to let the caller of PrintMembers to deal with the returned bool and using it to print the separators. We do that for printing the comma separator; the space separator should be no different.
In reply to: 476650024 [](ancestors = 476650024,476188140,476183479)
|
|
||
| // this.print(builder); | ||
| block.Add(F.ExpressionStatement(F.Call(F.This(), _printMethod, builderLocal))); | ||
| // if (this.print(builder)) builder.Append(" "); |
There was a problem hiding this comment.
print [](start = 28, length = 5)
Consider using this.PrintMembers explicitly in the comment. #Closed
Should Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordPrintMembers.cs:16 in 9fb3f77. [](commit_id = 9fb3f77, deletion_comment = False) |
I don't think we can change the name now, as RC1 locks the public contract for records. #ByDesign |
| // builder.Append(<name>); | ||
| // builder.Append(" = "); | ||
| // builder.Append((object)<value>); | ||
| // builder.Append((object)<value>); OR builder.Append(<value>.ToString()); |
There was a problem hiding this comment.
Consider clarifying when the OR applies. #Closed
|
Done review pass (commit 5) |
|
FYI, the correctness failure has been fixed in the base branch. |
ghost
added this to the
…-only-errors * upstream/master: (236 commits) Fix bug when "End statement" is used in single-line if (dotnet#47062) Solution asset cache refactoring (dotnet#46948) add specific tests to validate behavior between keys and snapshots Extract into separate files rename parameters rename parameters rename parameters rename parameters Add CancellationToken parameters to SyntaxTreeOptionsProvider Reuse nullable override checks for delegate conversions (dotnet#46953) Introduce warning for multiple entry points (sync + async) (dotnet#46832) Switch from throwing NotImplementedException and return E_NOTIMPL Delete Building for Core CLR.md (dotnet#47146) Adjust PrintMembers to avoid boxing and avoid extra space (dotnet#47095) Track asynchronous operation in InProcLanguageServer Use Task.FromCanceled where appropriate Apply suggestions from code review Address feedback Expose ParseOptions on generator context (dotnet#46919) Remove redundant statement in added tests ...
6 participants
Fixes #47093
Fixes #47092