<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Bill, <div><br></div><div>thanks for your comments. I'll respond to them individually. I've attached a new revision of the patch that addresses them. Patch built and tested against SVN 79487, with the additional attached fix that fixes an Intel table bug.</div><div><br></div><div>Sean</div><div><br><div><div>On 2009/08/18, at 0:57, Bill Wendling wrote:</div><blockquote type="cite"><div>0. Watch out for tabs!<br></div></blockquote><div><br></div>Fixed. Thanks.</div><div><br><blockquote type="cite"><div>1. Includes like this "#include <llvm/MC/MCInst.h>" should be in quotes, not angle brackets.<br></div></blockquote><div><br></div><div>Fixed.</div><br><blockquote type="cite"><div>2. In the "class Indenter", the "push" and "pop" methods check for over/under runs before proceeding. Should these be "asserts" instead?<br></div></blockquote><div><br></div><div>Yeah, I made them asserts as you suggested. I was thinking about maybe making them saturate, but honestly if we run out of space we should simply break and allow the developer to make the Indenter bigger. Maybe we should have another argument to the constructor specifying how large the buffer should be...?</div><br><blockquote type="cite"><div>3. In include/llvm/Support/MemoryObject.h, you changed all "uintptr_t" to "uint64_t". Is there a good reason for this? If not, please change them back.'<br></div></blockquote><div><br></div><div>Yes, there is. If you're disassembling in a memory space that's larger than yours (for example, on a computer where LLVM is built 32-bit but the source of the bytes is a 64-bit address space), then I don't want to be limited by the local process's notion of uintptr_t.</div><br><blockquote type="cite"><div>4. If you have a class that's completely contained within a .cpp file, go ahead and define it in an anonymous namespace and mark it "VISIBILITY_HIDDEN". Like this:<br><br>namespace {<br>class VISIBILITY_HIDDEN StringMemoryObject : public MemoryObject {<br>...<br></div></blockquote><div><br></div><div>I used VISIBILITY_HIDDEN for both StringMemoryObject and X86DisassemblerEmitter::X86DEBackend.</div><div>I wrapped StringMemoryObject in an anonymous namespace.</div><div>I didn't wrap X86DEBackend because it's already nested.</div><div><br></div><blockquote type="cite"><div>5. In "readBytes" in StringMemoryObject, you're returning a "-1", but the method's return type is "uint64_t". Do you *really* want to return 0xfffffffffffU, or is "-1" a sentinel value of some sort? If the latter, perhaps changing the return type to "int64_t" instead. If the former, then changing the return to "-1U" to make it explicit.<br></div></blockquote><div><br></div><div>-1 is a sentinel value. I changed readBytes to return an int and accept a uint64_t* that it fills in if it's non-NULL.</div><br><blockquote type="cite"><div>6. The use of "#include <iostream>" is strictly forbidden. Please remove it wherever you find it (HexDisassembler.h for one).<br></div></blockquote><div><br></div><div>Yeah, I guess those static initializers aren't so great. Removed.</div><br><blockquote type="cite"><div>7. Look for opportunities to use the LLVM small container classes. For instance, the "fOperands" ivar in RecognizableInsn.</div></blockquote><div><br></div><div>I looked at my uses of std::vector but they wouldn't really benefit from the good performance of SmallVector for small values of N.</div><div><br></div>- fInstructions in X86Disassembler.h is not a good candidate for this, because the number of instructions could potentially be quite large.</div><div>- numberedInstructions in X86DisassemblerEmitter.cpp is not a good candidate because there are upwards of a thousand instructions in the x86 table.</div><div>- fInstructionSpecifiers in X86DisassemblerTables.h is not a good candidate either... the number is (again) very large.</div><div>- fOperands is not a good candidate because is never actually created. It is always set to point to CodeGenInstruction::OperandList.</div><div><br><blockquote type="cite"><div>8. While we're on that, please use "Instruction" or "Instr" instead of "Insn" for consistency's sake.<br></div></blockquote><div><br></div><div>Fixed. Thanks for the heads-up.</div><br><blockquote type="cite"><div>9. You have types called "operandType_t", etc. We normally don't use the "_t" suffix for types. Perhaps renaming them.<font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#144FAE"><br></font></font></div></blockquote><div><br></div>Fixed. Now for example operandType_t is OperandType.<br><br><blockquote type="cite"><div>10. In the "static inline bool outranks(instructionContext_t upper, instructionContext_t lower)", it would be good to assert that upper and lower are strictly less than IC_max.<br></div></blockquote><div><br></div><div>Asserted.</div><br><blockquote type="cite"><div>11. In several places, you put 'default: assert(0 && "Badness happens here!");" at the end of a switch statement. When assertions are turned off, I believe that this could emit warnings. It would be better to put it as the first case of the "switch". And maybe use the new "llvm_unreachable()" macro as well.<br></div></blockquote><div><br></div>All right, I moved them and used llvm_unreachable(). In the C code, where I can't safely #include "llvm/Support/ErrorHandling.h," I declared a macro that does the same thing.</div><div><br><blockquote type="cite"><div>12. You use "std::endl" a lot. This is discouraged by the coding standard:<br><br><span class="Apple-tab-span" style="white-space:pre"> </span><a href="http://llvm.org/docs/CodingStandards.html#ll_avoidendl">http://llvm.org/docs/CodingStandards.html#ll_avoidendl</a><br></div></blockquote><div><br></div><div>Purged.</div><div><br></div><blockquote type="cite"><div>13. You use "std::ostream" a lot. Would it be appropriate to use "raw_ostream" instead?<br></div></blockquote><div><br></div><div>To make sure that the table definition file looks pretty, I use <iomanip>, and none of those functions work with raw_ostreams. I actually only create three std::ostringstreams – during table generation – so for now I'm going to leave that the way it is.</div><div><br></div><blockquote type="cite"><div>14. In DisassemblerTables::setTableFields, you emit things to "std::cerr". Use "errs()" instead. Or, better yet, "llvm_report_error". :-)<br></div></blockquote><div><br></div><div>Fixed to use errs().</div><br><blockquote type="cite"><div>15. Prefer pre-increment/decrement for variables:<br><br><span class="Apple-tab-span" style="white-space:pre"> </span><a href="http://llvm.org/docs/CodingStandards.html#micro_preincrement">http://llvm.org/docs/CodingStandards.html#micro_preincrement</a><br></div></blockquote><div><br></div>Learned a new one there. Thanks!<br><br><blockquote type="cite"><div>16. In X86RecognizableInsn.cpp, you use the large macro: HANDLE_OPERAND. Could you make it an inlined function instead? It could take a pointer to a member function for the method it wants to call. Debugging large macros is impossible. :)<font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#144FAE"><br></font></font></div></blockquote><div><br></div>I factored the code out into a function. There are still macros to keep things pretty, but they just wrap the function.</div><div><br><blockquote type="cite"><div>17. I'm confused by the C files in lib/Target/X86. Why aren't they C++ files?<br></div></blockquote><div><br></div><div>The reason is that I am aware of clients that need disassembly but can't link in C++ code, so for those clients I've separated out a C core. They won't get the benefits of the MCInst, but that's their problem :)</div><br><blockquote type="cite"><div>Okay. That's all I found for now. :-) I haven't looked at the algorithm too much, though it seems fairly straight-forward. I'm sure that since you're continually testing it, that it's of high quality. When you do get around to submission, we will want all of the tests you've been running to be added to the test/ directory.<br><br>Thanks!<br>-bw<br></div></blockquote></div><br></div><div>Sean</div><div><br></div><div></div></body></html>