<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On May 17, 2013, at 1:36 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br> Hi Ahmed,<br><br> This is going in the right direction.<br> Thanks for working on this.<br><br> I made a few comments inline, I let you comment/fix those.<br><br> In my opinion, the diff would have been simpler if you did not change the order of the arguments of tryAddingSymbolicOperand.<br> What was the rational for changing that order?<br><br> Generally speaking the patch looks good to me, but I am concerned about the feature we lose for ARM.<br><br> Jim, what do you think?<br><br></div></blockquote><div dir="auto"><br></div><div dir="auto">Yes, it’s very important to not lose the handling of the low/high ARM relocations. Their variant kinds should definitely stay in the target-specific code, as they have no relation to anything but ARM. In general, the symbolizer needs to be able to interface to target specific code to handle things like this.</div><div dir="auto"><br></div><div dir="auto">-Jim</div><div dir="auto"><br></div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>================<br>Comment at: lib/MC/MCDisassembler.cpp:26<br>@@ +25,3 @@<br>+ assert(ctx != 0 && "No MCContext given for symbolic disassembly");<br>+ setSymbolizer(new MCExternalSymbolizer(*ctx, getOpInfo, symbolLookUp,<br>+ disInfo));<br>----------------<br>I guess that this method is called only once, but in case, there is the delete counterpart of this new?<br><br>================<br>Comment at: lib/MC/MCExternalSymbolizer.cpp:117<br>@@ +116,3 @@<br>+ // variantkinds to MCSymbolRefExpr?<br>+<br>+ //if (SymbolicOp.VariantKind == LLVMDisassembler_VariantKind_ARM_HI16)<br>----------------<br>As you noted it in your initial comment, this is a regression compared to what we had.<br>ARM is an important target and I am not sure this is acceptable.<br><br><br><a href="http://llvm-reviews.chandlerc.com/D801">http://llvm-reviews.chandlerc.com/D801</a></div></blockquote></div><br></body></html>