Relocate recursion detection logic. #27
Conversation
This commit removes the recursion detection logic from `visitor.visitMap()`, `visitor.visitPtr()`, and `visitor.visitSlice()` methods and places it on the top of `visitor.visit()` method to have a single point of recursion detection and recursion marker printing. Adding `Value.CanNil()` method is required to detect whether it is safe to call `Value.IsNil()` on all possible kinds of values without causing the code to panic. Also an if branch was placed inside the `visitor.enter()` method to exit the method if the value is an empty interface. That was required to keep the interface rendering tests from failing and to keep the behaviour backward-compatible.
danilvpetrov
force-pushed
the
relocate-recursive-detection
branch
from
ba37971 to
24e7647
Compare
visitor.go
Outdated
|
|
||
| // If the value is an empty interface, return false as there is a specific | ||
| // case for rendering empty interfaces. | ||
| if v.DynamicType.Kind() == reflect.Interface && v.Value.IsNil() { |
There was a problem hiding this comment.
If this if is removed, is the output still correct? Could we remove this, and let the enter() function handle rendering the nil interfaces, and remove visitInterface()?
There was a problem hiding this comment.
We can definitely do that. However we still need this condition from visitor.visitInterface() to keep the tests as they are.
There was a problem hiding this comment.
Hrm, I guess since there is still interface-specific logic it belongs in a visitInterface() method, but something about having to do this check twice feels wrong, but I haven't come up with a "better" way yet.
There was a problem hiding this comment.
I think now that this has been made more generic, it probably shouldn't be responsible for nil rendering at all, only for the recursion. Which means we should let the nil rendering fall through to the filters and main switch statement regardless of the type (ie, not just interfaces). Does that make sense?
There was a problem hiding this comment.
Yes, definitely it makes sense. I'll commit more changes towards that direction soon.
There was a problem hiding this comment.
Ok, my last commit is an attempt to let only a single purpose for visitor.enter() method - recursion detection and recursive marker rendering.
I had to replace Value.canNil() with Value.canPointer() method since visitor.enter() no longer needs to check the value for nil, but it still needs to check if we can safely acquire a pointer from the value.
Co-Authored-By: James Harris <[email protected]>
The purpose of this commit is to refactor `visitor.enter()` method to narrow down its purpose to concurrency detection only. Previous nil value rendering was relocated to the types that specifically require nil rendering (ptr, slice, and map). Nil interface rendering was left intact as it requires some specifics when rendering ambiguous static types. As the remaining logic in `visitor.enter` now requires to receive a pointer from the given type, `Value.canNil()` method was replaced with `Value.canPointer()` method to check if it is safe to acquire a pointer from a value.
Co-Authored-By: James Harris <[email protected]>
This PR removes the recursion detection logic from
visitor.visitMap(),visitor.visitPtr(), andvisitor.visitSlice()methods and places it on the top ofvisitor.visit()method to have a single point of recursion detection and recursion marker printing.This PR is a precursor to #26 as
sync.Mapfilter requires recursion detection when renderingsync.Mapelements.