Skip to content

Commit fef5e5b

Browse files
committed
refactor(goldmark): finish the fieldorder dedup, share assertOwnFieldsPointersBeforeScalars
Round-2 code review flagged that the earlier fieldorder extraction only moved isPointerish (the smaller helper) into the shared package, leaving assertOwnFieldsPointersBeforeScalars duplicated verbatim in both pkg/goldmark/ast/structlayout_test.go and pkg/goldmark/parser/structlayout_test.go -- and the two copies had already drifted (one carried an extra comment sentence the other lacked), which is exactly the divergence sharing code is meant to prevent. Move it into fieldorder as AssertPointersBeforeScalars(t testing.TB, ...). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
1 parent 16ba274 commit fef5e5b

3 files changed

Lines changed: 30 additions & 48 deletions

File tree

pkg/goldmark/ast/structlayout_test.go

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -10,31 +10,10 @@ import (
1010
"testing"
1111

1212
"github.com/jeduden/mdsmith/pkg/goldmark/internal/fieldorder"
13-
"github.com/stretchr/testify/assert"
1413
)
1514

16-
// assertOwnFieldsPointersBeforeScalars checks that, among typ's
17-
// fields starting at index skip (use 1 to skip an embedded base
18-
// struct), no scalar (non-pointerish) field is followed by a
19-
// pointerish one.
20-
func assertOwnFieldsPointersBeforeScalars(t *testing.T, typ reflect.Type, skip int) {
21-
t.Helper()
22-
seenScalar := false
23-
for i := skip; i < typ.NumField(); i++ {
24-
f := typ.Field(i)
25-
if fieldorder.IsPointerish(f.Type.Kind()) {
26-
assert.Falsef(t, seenScalar,
27-
"field %s (%s, pointer-ish) is declared after a scalar field; "+
28-
"pointer fields should precede scalars to shrink GC ptrdata",
29-
f.Name, f.Type)
30-
} else {
31-
seenScalar = true
32-
}
33-
}
34-
}
35-
3615
func TestAutoLink_PointerFieldsBeforeScalars(t *testing.T) {
3716
// Index 0 is the embedded BaseInline; only AutoLink's own
3817
// declared fields are checked here.
39-
assertOwnFieldsPointersBeforeScalars(t, reflect.TypeOf(AutoLink{}), 1)
18+
fieldorder.AssertPointersBeforeScalars(t, reflect.TypeOf(AutoLink{}), 1)
4019
}

pkg/goldmark/internal/fieldorder/fieldorder.go

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,10 @@
77
// copy of the same reflection logic.
88
package fieldorder
99

10-
import "reflect"
10+
import (
11+
"reflect"
12+
"testing"
13+
)
1114

1215
// IsPointerish reports whether a field's kind carries at least one
1316
// pointer word that the GC must scan (a pointer, interface, slice,
@@ -20,3 +23,25 @@ func IsPointerish(k reflect.Kind) bool {
2023
}
2124
return false
2225
}
26+
27+
// AssertPointersBeforeScalars checks that, among typ's fields
28+
// starting at index skip (use 1 to skip an embedded base struct), no
29+
// scalar (non-pointerish) field is followed by a pointerish one. A
30+
// pointerish field declared after a scalar field extends the struct's
31+
// GC ptrdata past that scalar for no reason.
32+
func AssertPointersBeforeScalars(t testing.TB, typ reflect.Type, skip int) {
33+
t.Helper()
34+
seenScalar := false
35+
for i := skip; i < typ.NumField(); i++ {
36+
f := typ.Field(i)
37+
if IsPointerish(f.Type.Kind()) {
38+
if seenScalar {
39+
t.Errorf("field %s (%s, pointer-ish) is declared after a scalar field; "+
40+
"pointer fields should precede scalars to shrink GC ptrdata",
41+
f.Name, f.Type)
42+
}
43+
} else {
44+
seenScalar = true
45+
}
46+
}
47+
}

pkg/goldmark/parser/structlayout_test.go

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -13,41 +13,19 @@ import (
1313
"testing"
1414

1515
"github.com/jeduden/mdsmith/pkg/goldmark/internal/fieldorder"
16-
"github.com/stretchr/testify/assert"
1716
)
1817

19-
// assertOwnFieldsPointersBeforeScalars checks that, among typ's
20-
// fields starting at index skip (use 1 to skip an embedded base
21-
// struct), no scalar (non-pointerish) field is followed by a
22-
// pointerish one. A pointerish field declared after a scalar field
23-
// extends the struct's GC ptrdata past that scalar for no reason.
24-
func assertOwnFieldsPointersBeforeScalars(t *testing.T, typ reflect.Type, skip int) {
25-
t.Helper()
26-
seenScalar := false
27-
for i := skip; i < typ.NumField(); i++ {
28-
f := typ.Field(i)
29-
if fieldorder.IsPointerish(f.Type.Kind()) {
30-
assert.Falsef(t, seenScalar,
31-
"field %s (%s, pointer-ish) is declared after a scalar field; "+
32-
"pointer fields should precede scalars to shrink GC ptrdata",
33-
f.Name, f.Type)
34-
} else {
35-
seenScalar = true
36-
}
37-
}
38-
}
39-
4018
func TestDelimiter_PointerFieldsBeforeScalars(t *testing.T) {
4119
// Index 0 is the embedded ast.BaseInline; only Delimiter's own
4220
// declared fields are checked here.
43-
assertOwnFieldsPointersBeforeScalars(t, reflect.TypeOf(Delimiter{}), 1)
21+
fieldorder.AssertPointersBeforeScalars(t, reflect.TypeOf(Delimiter{}), 1)
4422
}
4523

4624
func TestLinkLabelState_PointerFieldsBeforeScalars(t *testing.T) {
47-
assertOwnFieldsPointersBeforeScalars(t, reflect.TypeOf(linkLabelState{}), 1)
25+
fieldorder.AssertPointersBeforeScalars(t, reflect.TypeOf(linkLabelState{}), 1)
4826
}
4927

5028
func TestFenceData_PointerFieldsBeforeScalars(t *testing.T) {
5129
// fenceData has no embedded base struct, so every field is checked.
52-
assertOwnFieldsPointersBeforeScalars(t, reflect.TypeOf(fenceData{}), 0)
30+
fieldorder.AssertPointersBeforeScalars(t, reflect.TypeOf(fenceData{}), 0)
5331
}

0 commit comments

Comments
 (0)