<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Sep 3, 2009, at 5:25 PM, Sean Callanan wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>I was away doing other things for a while, but I have an API patch separated out, which (in addition to being much smaller than past megapatches) corrects two issues Chris identified in his most recent set of patches:</div><div><br></div><div>- First, it makes the API a good deal simpler. Now, you can instantiate a single MCDisassembler and, each time you want an instruction disassembled, you can simply pass it a MemoryRegion, an offset, and an MCInst to populate.</div><div><br></div><div>- Second, it adds MCDisassembler to the list of things you can get from a TargetMachine, so you don't have to #include something from lib/Target/X86 just to use the X86 disassembler.</div><div><br></div><div>Please let me know what you think of this API. Thanks for your time.</div></div></blockquote><div><br></div></div>Hi Sean,<div><br></div><div>Sorry for the delay, this got buried in my inbox.</div><div><br></div><div>This API looks great, please commit it with a couple of minor tweaks:</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+++ include/llvm/MC/MCDisassembler.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div><div><font class="Apple-style-span" face="Menlo" size="2"><span class="Apple-style-span" style="font-size: 10px;"><br></span></font></div><div><font class="Apple-style-span" face="Menlo" size="2"><span class="Apple-style-span" style="font-size: 10px;"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+#include "llvm/MC/MCInst.h"</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+#include "llvm/Support/MemoryObject.h"</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+#include "llvm/Support/raw_ostream.h"</div><div><br></div><div>Please forward declare these classes instead of #including their headers.</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+ virtual bool getInstruction(MCInst& instr,</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+ uint64_t& size,</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+ MemoryObject ®ion,</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+ uint64_t address,</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+ raw_ostream &vStream) const = 0;</div><div><br></div><div>"MCInst &instr", likewise with size. Can "region" be passed in by const reference?</div><div><br></div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+++ lib/MC/MCDisassembler.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">@@ -0,0 +1,20 @@</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+//===-- lib/MC/MCDisassembler.h - Disassembler interface --------*- C++ -*-===//</div><div><br></div><div>Please fix the comment line (.h -> .cpp)</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+namespace llvm {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+MCDisassembler::MCDisassembler() {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+}</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Menlo; ">+ </div><div><br></div><div>In the .cpp files, please change them to 'using namespace llvm;' at the top to avoid nesting the entire file in a namespace.</div><div><br></div><div><br></div><div>Thanks Sean,</div><div><br></div><div>-Chris </div><div><br></div></div></div></div></span></font></div></div></body></html>