Cleaning up the doc tree #445
Conversation
belav
force-pushed
the
clean-up-doc-tree
branch
3 times, most recently
from
3e6a10c to
483d684
Compare
Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/BaseMethodDeclaration.cs
Outdated
Show resolved
Hide resolved
|
|
||
| if (arguments.Any()) | ||
| { | ||
| docs.Add( |
There was a problem hiding this comment.
Why is this change needed?
I'm not saying I prefer the old code over this, but I would like to understand why you're doing this.
Is there some penalty for having Doc.Null? If yes, wouldn't it be easier to filter them out in Doc.Concat(), causing them to be removed everywhere?
There was a problem hiding this comment.
There is probably some minor overhead to having Doc.Null in a Doc.Concat. The main goal of this was to clean some of them out of the displayed doc tree so it was easier to see what was going in with methods.
I always assumed stripping them out of the List<Doc> inside of Concat wouldn't be worth it and finally decided to benchmark it. Stripping them out does cause formatting to be ~2.4% slower and take ~2.9% more ram, at least on this very large file I use for benchmarking.
Maybe I should just modify the playground site to not show Doc.Null, or have an option to not show it.... that seems way easier. It does have some slight risk that the Doc.Null is affecting the formatting, but doesn't show in the doc tree.
belav
force-pushed
the
clean-up-doc-tree
branch
from
cefa7c4 to
c9db724
Compare
This PR cleans up the doc that we generate, which helps when trying to understand how code is being formatted. It probably also has some minor performance gains.