Terminal.Gui
Terminal.Gui copied to clipboard
gui-cs
FrameToScreen/BoundsToScreen work, but are not correct
In #3158 I found the logic I put in these apis is not actually correct. It works with current Frame functionality but only by accident.
This issue is to help me remember this needs to be fixed.
The solve will be to make FrameToScreen recursive vs using a while loop.
More unit tests are required in CoordinateTests.
Just a thought:
Remember that the csharp compiler cannot perform tail call optimization, so use of recursion usually results in slower code than the equivalent iterative approach, which is able to be aggressively optimized. The recursive code will always, at minimum, involve extra stack frames, and runs the risk of stack overflow, though that exception is not likely to occur in this case since it shouldn't typically go that deep. But it still incurs the cost of a stack frame push and method call.
Recursion for things like this can be clearer, but just something to be aware of.
If using recursion, be sure any value parameters in recursive calls are passed by reference, as that does make a difference in recursion and what the compiler can optimize.
Yes it's passed by reference and normally this kind of calculation use recursion which is the preferable.
Yep, I'm aware of that. I'm pointing out that, specifically, the C# compiler doesn't emit good code for recursion, even though the relevant opcodes are available in IL.
It's an unfortunate implementation detail of the compiler and has been a long-standing issue, with many proposals around it, for many years.
One way to make it better is to implement your recursion using local functions, though it still will never emit the .tail opcode.
It's important for a library to be conservative with the stack, as we don't know how deep we may already be when invoking an operation.
Again, though, just something to be aware of.
Personally, I prefer recursion when it makes sense, too, but there IS a cost, specifically when using C#.
This recursive method is accurate to get conditions where must have an end, otherwise is a StackOverflowException
Yes.
It's not a question of accuracy.
It's a notice of performance considerations due to implementation details of the C# compiler and debugging difficulty, if an exception (of any kind - not just a stack overflow) happens in a recursive method, in C#, specifically. Your stack trace will have every recursive call on it, and you have to unwind it yourself, since the compiler won't emit the .tail opcode and the JITer will just happily do as it's told, to its detriment.
In situations where it matters, I usually write the recursive method, and then let ReSharper turn it into an iterative implementation, and just keep the recursive code in a comment block in case anything needs to be changed later. Then I get the benefit of being able to write the nice recursive code, and the performance of the iterative code.
Cool, I've never used that Resharper feature. Didn't know it existed.
I'll give it at try when I tackle this issue.
Normally when the stack overflow exception throw it's in situations where the values passed into parameters and the return values are always the same for about 1000 times, more or less. So that would an coincidence or must probably for sure a bug 😃
Cool, I've never used that Resharper feature. Didn't know it existed.
I'll give it at try when I tackle this issue.
Yeah it's pretty nifty.
Though it won't offer it if it is complex enough that it can't figure out how to do it, so it's not available in 100% of situations.
Normally when the stack overflow exception throw it's in situations where the values passed into parameters and the return values are always the same for about 1000 times, more or less. So that would an coincidence or must probably for sure a bug 😃
Right. Infinite recursion bugs will of course throw it.
You can encounter it outside of those scenarios if you enter a non-trivial recursive method from a point in the application where the stack is already deep, as well, which is why it's just polite for libraries to try to avoid it when possible.
Now, if MS ever makes the compiler emit .tail opcodes, the whole problem will go away and it will cease to matter.
In this application, it's unlikely for this particular method to frequently have to traverse a tree with a depth more than single digits (amusingly, except maybe for TreeView), so it's probably not a big deal. It's just the first time someone mentioned recursion explicitly since I've had eyes on things, so it was mostly an FYI.
I believe this is all fixed in
- https://github.com/gui-cs/Terminal.Gui/pull/3278
Probably? 🤔
I guess if I notice anything still weird when I get back to stuff dependent on that, I'll file charges or whatever. 😛
Probably? 🤔
I guess if I notice anything still weird when I get back to stuff dependent on that, I'll file charges or whatever. 😛
https://github.com/gui-cs/Terminal.Gui/pull/3295 adds a bunch of unit tests that prove this stuff now finally, actually, does work as advertised.