Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(667)

Issue 149400043: code review 149400043: cmd/objdump: use cmd/internal/objfile (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 2 months ago by rsc
Modified:
9 years, 1 month ago
Reviewers:
r, gobot, brainman, iant
CC:
r, iant, golang-codereviews
Visibility:
Public.

Description

cmd/objdump: use cmd/internal/objfile This removes a bunch of ugly duplicate code. The end goal is to factor the disassembly code into cmd/internal/objfile too, so that pprof can use it, but one step at a time.

Patch Set 1 #

Patch Set 2 : diff -r 3f6f21d480eecb6df95c4e3652313daf0332788d https://code.google.com/p/go/ #

Patch Set 3 : diff -r abf51cd3e676e6c3f29bb57bd976ed3458f640fc https://code.google.com/p/go/ #

Patch Set 4 : diff -r abf51cd3e676e6c3f29bb57bd976ed3458f640fc https://code.google.com/p/go/ #

Total comments: 3

Patch Set 5 : diff -r 7c8e19a0c04ace946cf71bd0fbec3997be652fb5 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+154 lines, -568 lines) Patch
M src/cmd/internal/objfile/elf.go View 1 2 2 chunks +25 lines, -0 lines 0 comments Download
M src/cmd/internal/objfile/goobj.go View 1 2 1 chunk +12 lines, -0 lines 0 comments Download
M src/cmd/internal/objfile/macho.go View 1 2 1 chunk +24 lines, -0 lines 0 comments Download
M src/cmd/internal/objfile/objfile.go View 1 2 2 chunks +10 lines, -0 lines 0 comments Download
M src/cmd/internal/objfile/pe.go View 1 2 2 chunks +31 lines, -0 lines 0 comments Download
M src/cmd/internal/objfile/plan9obj.go View 1 2 2 chunks +22 lines, -0 lines 0 comments Download
R src/cmd/objdump/Makefile View 1 1 chunk +0 lines, -10 lines 0 comments Download
R src/cmd/objdump/elf.go View 1 2 1 chunk +0 lines, -65 lines 0 comments Download
R src/cmd/objdump/macho.go View 1 2 1 chunk +0 lines, -77 lines 0 comments Download
M src/cmd/objdump/main.go View 1 2 6 chunks +30 lines, -247 lines 0 comments Download
R src/cmd/objdump/pe.go View 1 2 1 chunk +0 lines, -99 lines 0 comments Download
R src/cmd/objdump/plan9obj.go View 1 2 1 chunk +0 lines, -70 lines 0 comments Download

Messages

Total messages: 11
rsc
Hello r (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
9 years, 1 month ago (2014-10-29 03:57:32 UTC) #1
r
LGTM
9 years, 1 month ago (2014-10-29 04:04:35 UTC) #2
brainman
https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/pe.go File src/cmd/internal/objfile/pe.go (right): https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/pe.go#newcode193 src/cmd/internal/objfile/pe.go:193: // Look in symbol table for telltale rt0 symbol. ...
9 years, 1 month ago (2014-10-29 06:04:40 UTC) #3
rsc
https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/pe.go File src/cmd/internal/objfile/pe.go (right): https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/pe.go#newcode193 src/cmd/internal/objfile/pe.go:193: // Look in symbol table for telltale rt0 symbol. ...
9 years, 1 month ago (2014-10-29 14:04:08 UTC) #4
iant
https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/pe.go File src/cmd/internal/objfile/pe.go (right): https://codereview.appspot.com/149400043/diff/60001/src/cmd/internal/objfile/pe.go#newcode193 src/cmd/internal/objfile/pe.go:193: // Look in symbol table for telltale rt0 symbol. ...
9 years, 1 month ago (2014-10-29 15:15:59 UTC) #5
rsc
Thanks. I don't know why I went looking on the internet instead of reading our ...
9 years, 1 month ago (2014-10-29 15:17:36 UTC) #6
iant
LGTM
9 years, 1 month ago (2014-10-29 15:21:44 UTC) #7
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=b3fb2d6e0e99 *** cmd/objdump: use cmd/internal/objfile This removes a bunch of ugly duplicate ...
9 years, 1 month ago (2014-10-29 22:07:30 UTC) #8
brainman
On 2014/10/29 14:04:08, rsc wrote: > What if the optional header is not present? I ...
9 years, 1 month ago (2014-10-29 22:34:49 UTC) #9
gobot
This CL appears to have broken the linux-arm-arm5 builder. See http://build.golang.org/log/2da7b7893978086ae426911bccfdee9a018531eb
9 years, 1 month ago (2014-10-29 22:57:09 UTC) #10
rsc
9 years, 1 month ago (2014-10-30 00:07:46 UTC) #11
I'll follow up with a CL to do what Ian suggested.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b