Skip to content

Commit a2e6b44

Browse files
committed
refactor(tests,builder): remove dev-process metadata and filler from comments
- Strip Task/Phase number references from function and test comments - Rewrite regression-guard language from past-tense to present-tense - Remove "for now" filler phrases - Update past-tense descriptions to present-tense design language Improves signal-to-noise ratio in developer-facing comments without changing technical content or intent. Signed-off-by: Martin Wimpress <code@wimpress.io>
1 parent 6549653 commit a2e6b44

10 files changed

Lines changed: 46 additions & 65 deletions

File tree

generated_test.go

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -285,8 +285,8 @@ func TestGeneratorCharVsUint8(t *testing.T) {
285285
list := ToCStr("test,foo,bar")
286286
defer list.Free()
287287

288-
// If this compiles, the char→C.char mapping is working correctly
289-
// (Previously this would fail: cannot use _Ctype_uint8_t as _Ctype_char)
288+
// If this compiles, the char→C.char mapping is correct. A uint8_t
289+
// regression would fail to compile: cannot use _Ctype_uint8_t as _Ctype_char.
290290
result, err := AVMatchList(name, list, ',')
291291
if err != nil {
292292
t.Fatalf("AVMatchList failed: %v", err)
@@ -443,8 +443,8 @@ func TestGeneratorTypePreservation(t *testing.T) {
443443
// This validates the Priority 1 enhancement that enables enum/struct const arrays
444444
func TestGeneratorConstArrayFields(t *testing.T) {
445445
t.Run("struct_const_array_AVRational", func(t *testing.T) {
446-
// Test that struct const array fields work (AVRational[N])
447-
// Previously skipped as "unknown const array"
446+
// Test that struct const array fields work (AVRational[N]).
447+
// Without const-array support these skip as "unknown const array".
448448
// Example: AVDetectionBBox.classify_confidences (AVRational[4])
449449

450450
// Create a detection bbox (would normally come from av_detection_bbox_alloc)
@@ -456,8 +456,8 @@ func TestGeneratorConstArrayFields(t *testing.T) {
456456
})
457457

458458
t.Run("enum_const_array", func(t *testing.T) {
459-
// Test that enum const array fields work (AVEnum[N])
460-
// Previously skipped as "unknown const array"
459+
// Test that enum const array fields work (AVEnum[N]).
460+
// Without const-array support these skip as "unknown const array".
461461
// Example: AVDOVIReshapingCurve.mapping_idc (AVDOVIMappingMethod[8])
462462

463463
// Verify the accessor compiles by taking the method value;
@@ -468,9 +468,9 @@ func TestGeneratorConstArrayFields(t *testing.T) {
468468
})
469469

470470
t.Run("byvalue_struct_array_helper", func(t *testing.T) {
471-
// Test that ToXArray helpers are generated for ByValue structs
472-
// Previously only generated for pointer structs
473-
// This enables the array field accessors above to work
471+
// Test that ToXArray helpers are generated for ByValue structs,
472+
// not only pointer structs. This enables the array field accessors
473+
// above to work.
474474

475475
// AVRational is a ByValue struct, should have ToAVRationalArray
476476
r1 := AVMakeQ(1, 2)

image_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import (
55
"unsafe"
66
)
77

8-
// TestImagePlaneRoundTrip proves the Task 2.1 plane/linesize helper converts a
8+
// TestImagePlaneRoundTrip proves the plane/linesize helper converts a
99
// four-plane set between Go slices and the fixed C [4] arrays without a cgocheck
1010
// abort. The plane pointers come from a C allocation (AVMalloc), mirroring how
1111
// real plane pointers originate from C-owned memory. Run under -race to exercise
@@ -43,7 +43,7 @@ func TestImagePlaneRoundTrip(t *testing.T) {
4343
}
4444
}
4545

46-
// TestAVImageAllocRoundTrip exercises the Task 2.2 alloc → fill → free path,
46+
// TestAVImageAllocRoundTrip exercises the alloc → fill → free path,
4747
// asserting dimensions and a balanced free.
4848
func TestAVImageAllocRoundTrip(t *testing.T) {
4949
const (

internal/builder/main.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -504,7 +504,7 @@ func printLibraryList(libs []*Library) {
504504
platform := getPlatformString(lib)
505505
buildSys := getBuildSystemString(lib)
506506

507-
// Get link libs display string (without embedded colors for now)
507+
// Link libs display string, without embedded colour codes
508508
linkLibsDisplay := getLinkLibsString(lib)
509509

510510
// Color code based on library type

internal/generator/generator.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -436,8 +436,8 @@ type Generator struct {
436436
}
437437

438438
// Gen runs the codegen pass. The skips collector is shared with Parse so a
439-
// single SkipCollector accumulates both parse-layer (Task 3.2) and codegen-layer
440-
// skips. A nil skips is tolerated: a fresh collector is allocated. The returned
439+
// single SkipCollector accumulates both parse-layer and codegen-layer skips.
440+
// A nil skips is tolerated: a fresh collector is allocated. The returned
441441
// collector is the active one (the argument when non-nil, otherwise the freshly
442442
// allocated one) so callers can read aggregated state without tracking it
443443
// separately.

internal/generator/generator_test.go

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,8 @@ func TestMarshalReturnSkip(t *testing.T) {
139139
}
140140

141141
// TestMarshalArgOutputPointerAllowlist pins the (function, parameter) allowlist
142-
// decision in marshalPointerArg. After Task 2.2 the allowlist is the only
143-
// output-pointer routing signal: a substring match like `width`, `size`, or
142+
// decision in marshalPointerArg. The allowlist is the only output-pointer
143+
// routing signal: a substring match like `width`, `size`, or
144144
// `_ptr` no longer keeps a parameter emittable on its own. This test asserts
145145
// that representative allowlisted pairs spanning the original heuristic
146146
// patterns still resolve to an emitted output pointer, and that near-miss
@@ -187,16 +187,15 @@ func TestMarshalArgOutputPointerAllowlist(t *testing.T) {
187187
}
188188
}
189189

190-
// TestMarshalArgPhase1OutputPointers pins the Phase 1 output-pointer table
191-
// additions: av_parse_time.timeval and av_get_output_timestamp.{dts,wall} are
192-
// pure-output int64_t* parameters now present in outputParams, so marshalArg
193-
// must emit them (skip == false) rather than record the former
194-
// "non-output primitive pointer" skip. Reintroducing the skip (by dropping the
195-
// table entry) flips skip to true here and trips the lowered skipCeiling in the
190+
// TestMarshalArgOutputPointers pins the output-pointer table entries:
191+
// av_parse_time.timeval and av_get_output_timestamp.{dts,wall} are pure-output
192+
// int64_t* parameters present in outputParams, so marshalArg must emit them
193+
// (skip == false) rather than record a "non-output primitive pointer" skip.
194+
// Dropping a table entry flips skip to true here and trips skipCeiling in the
196195
// generator run. av_expr_parse_and_eval.res is deliberately excluded: res is a
197196
// genuine output pointer, but the function stays skipped on its later
198197
// const char *const * params, so it never emits a callable binding.
199-
func TestMarshalArgPhase1OutputPointers(t *testing.T) {
198+
func TestMarshalArgOutputPointers(t *testing.T) {
200199
g := skipGen()
201200

202201
tests := []struct {
@@ -213,7 +212,7 @@ func TestMarshalArgPhase1OutputPointers(t *testing.T) {
213212
for _, tt := range tests {
214213
t.Run(tt.name, func(t *testing.T) {
215214
if _, ok := outputParams[tt.fnName][tt.argName]; !ok {
216-
t.Fatalf("outputParams[%q][%q] absent: Phase 1 table entry missing", tt.fnName, tt.argName)
215+
t.Fatalf("outputParams[%q][%q] absent: output-pointer table entry missing", tt.fnName, tt.argName)
217216
}
218217

219218
o := newFile()

internal/generator/main.go

Lines changed: 16 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -16,33 +16,20 @@ import (
1616
// skipCeiling caps the total skip-marker count a clean generator run is
1717
// allowed to record. It is an upper bound (`count > skipCeiling` trips), not
1818
// an exact equality, so it acts as a regression ceiling rather than a brittle
19-
// target.
19+
// target: it tolerates a handful of legitimate per-symbol skips a future header
20+
// bump might introduce while still catching wholesale degradation, such as an
21+
// allowlist regression that drops dozens of bindings.
2022
//
21-
// The 231 baseline is the post-Phase-1 count: the output-pointer table gained
22-
// av_parse_time and av_get_output_timestamp, dropping two non-output-pointer
23-
// skips from the former 233. An upper bound tolerates a handful of legitimate
24-
// per-symbol skips a future header bump might introduce while still catching
25-
// wholesale degradation (e.g. an allowlist regression that drops dozens of
26-
// bindings).
23+
// The current count is the residue of shapes the generator cannot safely emit:
24+
// int32 matrix-pointer functions, the AVExifEntry union field, and the
25+
// fixed-size struct-array fields in the DRM descriptors
26+
// (AVDRMFrameDescriptor.objects/layers, AVDRMLayerDescriptor.planes). Most are
27+
// covered by hand-written bindings; the skip marker records that the generator
28+
// declined them, not that the symbol is unavailable.
2729
//
28-
// Bumping this constant is a curation decision. A legitimate FFmpeg upgrade
29-
// that introduces new unemittable symbols requires an intentional bump
30-
// alongside the header update; do not raise it to silence a regression.
31-
//
32-
// Bumped 231 -> 239 for the Tier 1 header promotion (ac3_parser, adts_parser,
33-
// avdct, dirac, dv_profile, vorbis_parser, md5): the newly bound headers carry
34-
// a handful of per-symbol unemittable shapes. This is an intentional curation
35-
// decision accompanying the allowlist expansion, not silencing a regression.
36-
//
37-
// Bumped 239 -> 245 for the Tier 2 header promotion (exif, hwcontext_drm): six
38-
// new per-symbol unemittable shapes land as recorded skips — two int32
39-
// matrix-pointer functions and the AVExifEntry union field from exif.h, plus
40-
// three fixed-size struct-array fields in the DRM descriptors
41-
// (AVDRMFrameDescriptor.objects/layers, AVDRMLayerDescriptor.planes). These are
42-
// deferred to a later hand-written-binding commit; the curation policy accepts
43-
// them as tracked skips, not a regression. (smpte_436m.h was evaluated for the
44-
// same tier but dropped: its symbols are absent from the FFmpeg 8.1.1 static
45-
// lib and would break the link.)
30+
// Bumping this constant is a curation decision. An FFmpeg upgrade that binds new
31+
// headers carrying unemittable shapes needs an intentional bump alongside the
32+
// header update; do not raise it to silence a regression.
4633
const skipCeiling = 245
4734

4835
func main() {
@@ -74,13 +61,9 @@ func enforceSkipCeiling(total, ceiling int) error {
7461

7562
// run drives one end-to-end generator pass: parse the FFmpeg headers, apply
7663
// the in-tree fixups, and emit the five `*.gen.go` files. It returns the skip
77-
// collector populated during emission so callers (main, tests, the Task 3.4
78-
// ceiling check) can inspect every `skipped due to ...` decision the run made.
64+
// collector populated during emission so callers (main, the ceiling check,
65+
// tests) can inspect every `skipped due to ...` decision the run made.
7966
//
80-
// The function is the single extraction point Task 3.1 added so the run path
81-
// is testable without invoking the package binary. summaryOut is reserved for
82-
// callers that want non-stderr toolchain logging; today only the verbose flag
83-
// touches it via log.SetOutput.
8467
// It exists as a separate function so the whole run path is testable without
8568
// invoking the package binary.
8669
func run(args []string) (*SkipCollector, error) {
@@ -184,8 +167,8 @@ func manualBindingBySymbol(c *SkipCollector) map[string]string {
184167
}
185168

186169
// sortedUniqueSymbols returns the unique skipped symbol names in lexical
187-
// order. Used by the run summary and exposed for Task 3.4 to feed the same
188-
// list into the ceiling diagnostic when the cap trips.
170+
// order, feeding both the run summary and the ceiling diagnostic when the cap
171+
// trips.
189172
func sortedUniqueSymbols(c *SkipCollector) []string {
190173
if c == nil {
191174
return nil

internal/generator/parser.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -223,8 +223,8 @@ func sentinelParse() {
223223

224224
// Parse drives the libclang parse over the pinned FFmpeg headers. The skips
225225
// collector is the same instance Gen receives, so parse-layer unhandled-shape
226-
// skips (Task 3.2) land in the run-summary alongside codegen-layer skips. A nil
227-
// skips is tolerated: SkipCollector.Record is nil-safe.
226+
// skips land in the run-summary alongside codegen-layer skips. A nil skips is
227+
// tolerated: SkipCollector.Record is nil-safe.
228228
func Parse(skips *SkipCollector) *Module {
229229
p := &Parser{
230230
mod: &Module{

internal/generator/parser_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ import (
1212
)
1313

1414
// TestParseFunctionDuplicateAborts pins the invariant-violation panic at
15-
// parser.go:619 (duplicate function registration). Task 3.2 converted shape
16-
// panics to skip-with-reason but deliberately kept invariant panics fatal:
15+
// parser.go:619 (duplicate function registration). The parser converts shape
16+
// panics to skip-with-reason but deliberately keeps invariant panics fatal:
1717
// a duplicate registration means libclang misreported or the parser visited
1818
// the same FunctionDecl twice, so the bindings are untrustworthy and aborting
1919
// is correct. This test fabricates the duplicate by pre-seeding the module's

lib/fetch_test.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,9 +115,8 @@ func TestFindViaAPI_ReleaseSorting(t *testing.T) {
115115
// Last element should be the "highest"
116116
highest := sortedReleases[len(sortedReleases)-1]
117117

118-
// Note: lexicographic sorting means "lib-8.1.1.2" > "lib-8.1.1.10"
119-
// This is a known limitation of the current implementation
120-
// For now, we document the behavior
118+
// Note: lexicographic sorting means "lib-8.1.1.2" > "lib-8.1.1.10",
119+
// a known limitation of this sort.
121120
t.Logf("Sorted releases: %v", sortedReleases)
122121
t.Logf("Selected highest: %s", highest)
123122

samples_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import (
44
"testing"
55
)
66

7-
// TestAVSamplesAllocSilenceRoundTrip exercises the Task 2.3 alloc → silence → free
7+
// TestAVSamplesAllocSilenceRoundTrip exercises the alloc → silence → free
88
// path for a planar format, asserting a balanced free. Run under -race to exercise
99
// cgocheck on the plane-pointer array scratch.
1010
func TestAVSamplesAllocSilenceRoundTrip(t *testing.T) {

0 commit comments

Comments
 (0)