Simplify sequence point display#81104
Conversation
The string taken to control whether to use the inline sequence point display is never actually used other than being compared against `null`. Simplify that to just be a boolean flag.
The string passed to `sequencePoints` was only ever checked against `null`. Simplified to just passing a `bool`.
Instead of 2 boolean flags, which were inconsistently available, use a single enum to control sequence point visualization, with documentation about the differences in visualization behavior.
This was removed in dotnet#61064, and broke visualization of BasicBlocks. I've restored it and left a comment indicating its current use.
|
@dotnet/roslyn-compiler for review. Recommend commit-by-commit here for ease of reviewing mechanical changes: Commit 1 moves one of the string parameters to a bool. |
|
Thanks for improving this :-) I've found that even small improvements to our test helpers bring me a lot of joy since it removes friction in something we do so much of.
Just curious, did you use some automation to make this change? If so, consider sharing for educational purpose |
jcouv
left a comment
There was a problem hiding this comment.
LGTM Thanks (commit 4) with a clarifying question
Regex find/replace for most of it. Nothing special. |
|
Can we use AI to eliminate Minimal (convert all those tests that use it to Enhanced)? |
I've tried similar things recently, and the size of our test output is enough to kill any context windows I've tried. |
* upstream/main: Simplify sequence point display (dotnet#81104) [main] Update dependencies from dotnet/arcade (dotnet#81129) [main] Update dependencies from dotnet/arcade (dotnet#81105) Target typed conditional expression recovery (dotnet#81025) dotnet#81118: Fix dropdown in "Move static members" dialog (dotnet#81119) Don't emit DebuggerStepThroughAttribute for runtime async methods (dotnet#81035)
Right now, two different string parameters are used to control sequence point display. Neither string is used beyond null checks. I've simplified this to be just a single, more explanatory enum. Along the way, I noticed that BasicBlock's visualization is also broken, so I restored the private method that it used for that.