Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relocate recursion detection logic. #27

Merged
merged 5 commits into from
Jan 3, 2020
Merged

Relocate recursion detection logic. #27

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
24e7647
Relocate recursion detection to visitor.visit().
danilvpetrov Dec 29, 2019
c97f264
Revise Value.CanNil method comment.
danilvpetrov Dec 30, 2019
a9dd063
Unexport `visitor.CanNil()` method.
danilvpetrov Dec 30, 2019
3741e92
Let only concurrency detection in visitor.enter().
danilvpetrov Jan 2, 2020
19f4e00
Improve visitor.renderNil() comment.
danilvpetrov Jan 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions map.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,6 @@ import (
//
// TODO(jmalloc): sort numerically-keyed maps numerically
func (vis *visitor) visitMap(w io.Writer, v Value) {
if vis.enter(w, v) {
return
}
defer vis.leave(v)

if v.IsAmbiguousType() {
must.WriteString(w, v.TypeName())
}
Expand Down
5 changes: 0 additions & 5 deletions ptr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,6 @@ import (

// visitPtr formats values with a kind of reflect.Ptr.
func (vis *visitor) visitPtr(w io.Writer, v Value) {
if vis.enter(w, v) {
return
}
defer vis.leave(v)

if v.IsAmbiguousType() {
must.WriteByte(w, '*')
}
Expand Down
5 changes: 0 additions & 5 deletions slice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,5 @@ import (

// visitSlice formats values with a kind of reflect.Slice.
func (vis *visitor) visitSlice(w io.Writer, v Value) {
if vis.enter(w, v) {
return
}
defer vis.leave(v)

vis.visitArray(w, v)
}
13 changes: 13 additions & 0 deletions value.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,16 @@ func (v *Value) IsAnonymousType() bool {
func (v *Value) IsAmbiguousType() bool {
return v.IsAmbiguousDynamicType || v.IsAmbiguousStaticType
}

// CanNil reports if the Value can be nil as a zero value. When this method
danilvpetrov marked this conversation as resolved.
Show resolved Hide resolved
// returns true, it is safe to call IsNil() on the value without causing
// panicking.
func (v *Value) CanNil() bool {
danilvpetrov marked this conversation as resolved.
Show resolved Hide resolved
switch v.DynamicType.Kind() {
case reflect.Chan, reflect.Func, reflect.Interface,
reflect.Map, reflect.Ptr, reflect.Slice, reflect.UnsafePointer:
return true
default:
return false
}
}
17 changes: 16 additions & 1 deletion visitor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ func (vis *visitor) visit(w io.Writer, v Value) {
return
}

if vis.enter(w, v) {
return
}
defer vis.leave(v)

for _, f := range vis.filters {
if n := must.Must(f(w, v)); n > 0 {
return
Expand Down Expand Up @@ -79,6 +84,16 @@ func (vis *visitor) visit(w io.Writer, v Value) {
// It returns true if the value is nil, or recursion has occurred, indicating
// that the value should not be rendered.
func (vis *visitor) enter(w io.Writer, v Value) bool {
if !v.CanNil() {
return false
}

// 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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely do that. However we still need this condition from visitor.visitInterface() to keep the tests as they are.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely it makes sense. I'll commit more changes towards that direction soon.

Copy link
Member Author

@danilvpetrov danilvpetrov Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return false
}

marker := "nil"

if !v.Value.IsNil() {
Expand Down Expand Up @@ -113,7 +128,7 @@ func (vis *visitor) enter(w io.Writer, v Value) bool {
//
// It must be called after enter(v) returns true.
func (vis *visitor) leave(v Value) {
if !v.Value.IsNil() {
if v.CanNil() && !v.Value.IsNil() {
delete(vis.recursionSet, v.Value.Pointer())
}
}