Improving formatting of InvocationExpressions #444
Conversation
|
This is starting to get close, still lots of edge cases to check, lots of code to review after formatting with this, and some cleanup after the code "finished". |
… performance tweaks
Fixing bug with ConditionalGroup
Src/CSharpier/DocSerializer.cs
Outdated
| for (var x = 0; x < docs.Length; x++) | ||
| { | ||
| var printResult = PrintIndentedDocTree(docs[x]); | ||
| result += printResult; |
There was a problem hiding this comment.
You're doing string concat in a for loop. I know you've done a fair bit of benchmarks on this already.
Do you have a rule of thumb for when it's worth using a StringBuilder or even a pool of StringBuilders?
There was a problem hiding this comment.
DocSerializer is only used during testing, and it wasn't straightforward to switch over to a StringBuilder so I was just throwing code in it and not worrying about it.
But! I was motivated to figure it out after your comment. So now it uses a single StringBuilder.
| return doc switch | ||
| { | ||
| IHasContents hasContents => ContainsBreak(hasContents.Contents), | ||
| Concat concat => concat.Contents.Any(ContainsBreak), |
There was a problem hiding this comment.
How come Concat doesn't implement IHasContents?
Do they mean different things sematically?
There was a problem hiding this comment.
I think I have gotten confused and asked the same question.
Concat.Contents is a List<Doc>
IHasContents.Contents is a Doc
One of the two should probably get renamed.
Or.... maybe Concat shouldn't really exist, because it is really just a List<Doc>, but I don't know how feasible it is to make that work.
| or PredefinedTypeSyntax | ||
| or IdentifierNameSyntax | ||
| { | ||
| Identifier: { Text: { Length: <= 4 } } |
There was a problem hiding this comment.
pattern matching for the win! 🎉🎉🎉
| this | ||
| .CallMethod() | ||
| .CallMethod(); | ||
| vs |
There was a problem hiding this comment.
it would be clearer if you used "instead of" rather than "vs", to make it clearer which behavior is the result of not merging the first two groups.
| */ | ||
| private static bool ShouldMergeFirstTwoGroups(List<List<PrintedNode>> groups) | ||
| { | ||
| if (groups.Count < 2 || groups[0].Count != 1) |
There was a problem hiding this comment.
Should this also take into account the length of the first token?
e.g. should:
this
.CallMethod()
.CallMethod()
break differently than
someLongAssVariableName
.CallMethod()
.CallMethod()
There was a problem hiding this comment.
Lower down in the method handles short identifiers. This if statement just bails out early in some situations.
| @@ -131,14 +131,25 @@ public static Doc Print(CSharpSyntaxNode node) | |||
| ); | |||
| } | |||
|
|
|||
| if (typeParameterList != null) | |||
| if (typeParameterList != null && typeParameterList.Parameters.Any()) | |||
There was a problem hiding this comment.
nit: you can merge these two checks into one pattern: typeParameterList is { Count: > 0 }
closes #7