<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><base href="x-msg://40462/"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><br></div><div>In general, for things like this assert() is typical. It's assert(0 && "omg what happened?!") type things that are better off being llvm_unreachable(). That is, when there's real conditional that doesn't gate anything but the compile-time failure, an assert() is good. When it's just a "we should never get here" marker, llvm_unreachable() is preferred. In optimized builds, I suspect LLVM is smart enough to optimize everything away even in the former case since it'll end up being an empty compound statement body, so it's probably the same end result, it's just not the common idiom.</div><div><br></div><div>Other than that general observation, this looks good to me.</div><div><br></div><div>-Jim</div><div><br></div><div><br></div><div><div><div>On Jul 26, 2012, at 11:42 AM, "Kaylor, Andrew" <<a href="mailto:andrew.kaylor@intel.com">andrew.kaylor@intel.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><div class="WordSection1" style="page: WordSection1; "><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">The attached patch fixes a couple of problems in the handling of 32-bit relocations in 64-bit code in the RuntimeDyldELF class. In the first case, the assertion in the handler didn’t allow for the case of a positive 32-bit value with a R_X86_64_32S relocation. In addition to correcting that problem, I used named constants to make the code more readable here. In the second case, the magic values that were being used to check for a valid 32-bit value were missing a digit. I replaced them with named constants. I modified the code in both of these locations to use llvm_unreachable for the unexpected cases rather than assert, as this seems to be more consistent with LLVM coding practices.<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">-Andy<o:p></o:p></div></div><span><x86-64-32-relocation.patch></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" style="color: purple; text-decoration: underline; ">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" style="color: purple; text-decoration: underline; ">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br></div></body></html>