<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Apr 1, 2010, at 6:48 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; ">We talked about this a little off-list, and decided to have the disassembler return an error code in all cases, but only print a debug message on NDEBUG builds. The idea was to use the DEBUG macro and debugs() stream.<div><br></div><div>The attached patch implements these updates. Please let me know what you think.</div></div></blockquote></div><br><div>The patch generally looks good to me, remind me again why X86DisassemblerDecoder.c has to be a C file though?</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">- translateInstruction(instr, internalInstr);</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">- return true;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+ if (translateInstruction(instr, internalInstr))</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+ return false;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+ else</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+ return true;</div><div><font class="Apple-style-span" face="Inconsolata"><br></font></div><div><font class="Apple-style-span" face="Inconsolata">This can just be: return !<span class="Apple-style-span" style="font-family: Helvetica; "><span class="Apple-style-span" style="font-family: Inconsolata; font-size: 12px; ">translateInstruction(...</span></span></font></div><div><font class="Apple-style-span" face="Inconsolata"><br></font></div><div><font class="Apple-style-span" face="Inconsolata">However, it looks like <span class="Apple-style-span" style="font-size: 12px; ">translateInstruction can just return a bool instead of 0/-1. If you do this, please use true for error to follow other llvm precedent, and allowing the ! to go away. Likewise for translateRM etc.</span></font></div><div><font class="Apple-style-span" face="Inconsolata"><br></font></div><div><font class="Apple-style-span" face="Inconsolata"><br></font></div><div><font class="Apple-style-span" face="Inconsolata"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+ if (!(insn.eaBase != EA_BASE_sib && insn.eaBase != EA_BASE_sib64)) {</div><div><br></div><div>Please apply demorgan's law to simplify the condition.</div><div><br></div><div><br></div></font></div><div><font class="Apple-style-span" face="Inconsolata"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+ if (!(stackPos < 8)) {</div><div><br></div><div>Please simplify.</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+#ifndef NDEBUG</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+#define debug(s) do { x86DisassemblerDebug(__FILE__, __LINE__, s); } while (0);</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+#else</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+#define debug(s) do { } while (0);</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+#endif</div><div><br></div><div>The typical do/while(0) idiom doesn't include the trailing ; in the macro.</div><div><br></div><div>Otherwise, looks fine, please commit with changes,</div><div><br></div><div>-Chris</div><div><br></div></div></font></div><div><font class="Apple-style-span" face="Inconsolata"><br></font></div><div><font class="Apple-style-span" face="Inconsolata"><br></font></div></div></body></html>