From ea00461b17c7579d1c9aff2398953b61747ce642 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Wed, 11 Jun 2025 21:35:29 +0000 Subject: [PATCH] internal/trace: make Value follow reflect conventions A previous change renamed Value.Uint64 to Value.ToUint64 to accomodate string values. The method for a string value is then Value.ToString, while the method for a debug string (for example, for fmt) is just called String, as per fmt.Stringer. This change follows a request from Dominik Honnef, maintainer of gotraceui, to make Value follow the conventions of the reflect package. The Value type there has a method String which fulfills both purposes: getting the string for a String Value, and as fmt.Stringer. It's not exactly pretty, but it does make sense to just stick to convention. Change-Id: I55b364be88088d2121527bffc833ef03dbdb9764 Reviewed-on: https://go-review.googlesource.com/c/go/+/680978 Reviewed-by: Florian Lehner LUCI-TryBot-Result: Go LUCI Reviewed-by: Carlos Amedee --- src/cmd/trace/gen.go | 6 ++-- src/internal/trace/event.go | 5 ++++ src/internal/trace/gc.go | 2 +- src/internal/trace/testtrace/validation.go | 2 +- src/internal/trace/value.go | 34 +++++++++------------- 5 files changed, 23 insertions(+), 26 deletions(-) diff --git a/src/cmd/trace/gen.go b/src/cmd/trace/gen.go index 4455f83046..9cc22df1f6 100644 --- a/src/cmd/trace/gen.go +++ b/src/cmd/trace/gen.go @@ -283,11 +283,11 @@ func (g *globalMetricGenerator) GlobalMetric(ctx *traceContext, ev *trace.Event) m := ev.Metric() switch m.Name { case "/memory/classes/heap/objects:bytes": - ctx.HeapAlloc(ctx.elapsed(ev.Time()), m.Value.ToUint64()) + ctx.HeapAlloc(ctx.elapsed(ev.Time()), m.Value.Uint64()) case "/gc/heap/goal:bytes": - ctx.HeapGoal(ctx.elapsed(ev.Time()), m.Value.ToUint64()) + ctx.HeapGoal(ctx.elapsed(ev.Time()), m.Value.Uint64()) case "/sched/gomaxprocs:threads": - ctx.Gomaxprocs(m.Value.ToUint64()) + ctx.Gomaxprocs(m.Value.Uint64()) } } diff --git a/src/internal/trace/event.go b/src/internal/trace/event.go index 21f1569f43..f31412e35d 100644 --- a/src/internal/trace/event.go +++ b/src/internal/trace/event.go @@ -8,6 +8,7 @@ import ( "fmt" "iter" "math" + "strconv" "strings" "time" @@ -812,6 +813,10 @@ func (e Event) String() string { switch kind := e.Kind(); kind { case EventMetric: m := e.Metric() + v := m.Value.String() + if m.Value.Kind() == ValueString { + v = strconv.Quote(v) + } fmt.Fprintf(&sb, " Name=%q Value=%s", m.Name, m.Value) case EventLabel: l := e.Label() diff --git a/src/internal/trace/gc.go b/src/internal/trace/gc.go index f5e8fe79f2..46890e784d 100644 --- a/src/internal/trace/gc.go +++ b/src/internal/trace/gc.go @@ -103,7 +103,7 @@ func MutatorUtilizationV2(events []Event, flags UtilFlags) [][]MutatorUtil { if m.Name != "/sched/gomaxprocs:threads" { break } - gomaxprocs := int(m.Value.ToUint64()) + gomaxprocs := int(m.Value.Uint64()) if len(ps) > gomaxprocs { if flags&UtilPerProc != 0 { // End each P's series. diff --git a/src/internal/trace/testtrace/validation.go b/src/internal/trace/testtrace/validation.go index 3de1e1d4bd..5edcf3a5b2 100644 --- a/src/internal/trace/testtrace/validation.go +++ b/src/internal/trace/testtrace/validation.go @@ -135,7 +135,7 @@ func (v *Validator) Event(ev trace.Event) error { switch m.Value.Kind() { case trace.ValueUint64: // Just make sure it doesn't panic. - _ = m.Value.ToUint64() + _ = m.Value.Uint64() } case trace.EventLabel: l := ev.Label() diff --git a/src/internal/trace/value.go b/src/internal/trace/value.go index bf396b6a9e..fc2808e597 100644 --- a/src/internal/trace/value.go +++ b/src/internal/trace/value.go @@ -35,24 +35,27 @@ func (v Value) Kind() ValueKind { return v.kind } -// ToUint64 returns the uint64 value for a ValueUint64. +// Uint64 returns the uint64 value for a ValueUint64. // // Panics if this Value's Kind is not ValueUint64. -func (v Value) ToUint64() uint64 { +func (v Value) Uint64() uint64 { if v.kind != ValueUint64 { - panic("ToUint64 called on Value of a different Kind") + panic("Uint64 called on Value of a different Kind") } return v.scalar } -// ToString returns the uint64 value for a ValueString. -// -// Panics if this Value's Kind is not ValueString. -func (v Value) ToString() string { - if v.kind != ValueString { - panic("ToString called on Value of a different Kind") +// String returns the string value for a ValueString, and otherwise +// a string representation of the value for other kinds of values. +func (v Value) String() string { + if v.kind == ValueString { + return unsafe.String((*byte)(v.pointer), int(v.scalar)) + } + switch v.kind { + case ValueUint64: + return fmt.Sprintf("Value{Uint64(%d)}", v.Uint64()) } - return unsafe.String((*byte)(v.pointer), int(v.scalar)) + return "Value{Bad}" } func uint64Value(x uint64) Value { @@ -62,14 +65,3 @@ func uint64Value(x uint64) Value { func stringValue(s string) Value { return Value{kind: ValueString, scalar: uint64(len(s)), pointer: unsafe.Pointer(unsafe.StringData(s))} } - -// String returns the string representation of the value. -func (v Value) String() string { - switch v.Kind() { - case ValueUint64: - return fmt.Sprintf("Value{Uint64(%d)}", v.ToUint64()) - case ValueString: - return fmt.Sprintf("Value{String(%s)}", v.ToString()) - } - return "Value{Bad}" -} -- 2.50.0