<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Mar 20, 2011, at 11:53 AM, Frits van Bommel wrote:</div><br><blockquote type="cite"><div>You're not checking the return type here; see below.<br><br></div></blockquote><div><br></div>Fixed.</div><div><br></div><div><blockquote type="cite"><div>Fn.empty() is equivalent to Fn.isDeclaration(). Do you really want to<br>allow external destructors to be deleted?<br><br>(Note: you can allow them if they're readonly/readnone and nounwind)<font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#144FAE"><br></font></font></div></blockquote><div><br></div>Nope, I don't think we want them to be deleted. I added a FIXME for the readonly/readnone and nounwind case.</div><div><br></div><div><blockquote type="cite"><div>If the destructor is an infinite recursion at run-time, this turns it<br>into an infinite recursion at compile time...<br></div></blockquote><div><br></div>Fixed!</div><div><br><blockquote type="cite"><div><blockquote type="cite">+ } else if (isa<ReturnInst>(*I))<br></blockquote><blockquote type="cite">+ return true;<br></blockquote><blockquote type="cite">+ else<br></blockquote><blockquote type="cite">+ return false;<br></blockquote><br>I think you could allow arbitrary non-terminator instructions without<br>side-effects here too. These could be passed as parameters to the<br>calls, for instance.<br>In fact, that check could be moved up to filter out side-effect-free<br>function calls too.<br></div></blockquote><div><br></div>I intentionally wanted to make this as simple as possible to only catch what llvm-gcc and clang does.</div><div><br></div><div><blockquote type="cite"><div><br>According to the comment above, __cxa_atexit returns an int. If it has<br>any uses, this leaves them dangling.<br>Since it should be safe to pretend that registering an empty<br>destructor is successful, you should call<br>"CS->replaceAllUsesWith(Constant::getNullValue(CS.getType()));" before<br>erasing __cxa_atexit calls that have uses.<br>You should probably also either check for non-void return type in<br>FindCXAAtExit(), or skip the RAUW if the return type is void.<font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#144FAE"><br></font></font></div></blockquote><div><br></div>Added a check for an integer return type and used RUAW.</div><div><br><blockquote type="cite"><div>Also, note that 'CS.getInstruction()->' is equivalent to 'CS->' due to<br>operator overloading :).<br></div></blockquote></div><br><div>Doh! Fixed. (Everything is fixed in r127974). Thanks for the review!</div><div><br></div><div>- Anders</div></body></html>