Skip to content
This repository has been archived by the owner on Aug 30, 2019. It is now read-only.

Replace Trace []Span with []*Span #340

Merged
merged 1 commit into from
Dec 6, 2017
Merged

Conversation

LotharSee
Copy link

Another step before the global introduction of protobuf.

By using references it will simplify the code migration (as generated protobuf structs also include themselves by reference).

That should also avoid various struct copies.

Notes:

  • for model/trace_gen.go, I simply ran msgp -file=model/trace.go -marshal=false. That should be enough as trace_gen.go isn't a file we manually modified. I also checked to simply regenerate the file without the Trace change and I got exactly the same file as before so the generation method is good.
  • Quantize is now in-place.
  • The PR is based on the protobuf PR ; I'll merge them together once I've tested them enough.

@LotharSee LotharSee force-pushed the benjamin/trace-pointer branch from da2fcf3 to 4b0ccf7 Compare December 5, 2017 14:17
Copy link

@AlexJF AlexJF left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link

@ufoot ufoot left a comment

Choose a reason for hiding this comment

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

GTM

Rakefile Outdated

desc "Regenerate messagepack files"
task :msgp do
# Actually, don't use it since we modified manually the generated files for
Copy link

Choose a reason for hiding this comment

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

Nitpicking, but if we don't use this, a command-like example such as:

puts "don't use it since we modified manually the generated files"
puts "to run it manually: msgp -file=model/span.pb.go -marshal=false"

would do the job and get a higher probability not to disappear at next refactoring.

if err != nil {
return
for bzg := range *z {
if dc.IsNil() {
Copy link

Choose a reason for hiding this comment

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

OK, so that's the big difference I guess.

@LotharSee LotharSee force-pushed the benjamin/trace-pointer branch from 4b0ccf7 to 0067fb9 Compare December 6, 2017 12:36
@LotharSee LotharSee changed the base branch from benjamin/protobuf-span to master December 6, 2017 12:37
@LotharSee LotharSee merged commit 8df8d45 into master Dec 6, 2017
@dtilghman dtilghman deleted the benjamin/trace-pointer branch December 7, 2017 05:14
@palazzem palazzem added this to the 5.21.1 milestone Jan 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants