<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;">Hi Filip,<div><br></div><div>This seems generally fine. Just a couple of nitpicks.</div><div><br></div><div>Let’s make it explicit that it’s the host process string being retrieved since this will be used mainly in the MCJIT context and that it’s a triple being retrieved. "LLVMGetProcessTriple()” seems right to me.</div><div><br></div><div>Less important, but I’d prefer having the size argument to the function say that’s what it is, so something like, “BufferSize,” perhaps?</div><div><br></div><div><div><blockquote type="cite">+ * size_t targetSize = LLVMGetProcessTarget(0, 0);</blockquote></div><div><br></div><div>Use an explicit NULL here for the pointer value.</div><div><br></div><div><div><blockquote type="cite">"If TargetBytes isn't enough then the Target string is filled only up to TargetBytes; that is this function will never overflow that bound.”</blockquote></div></div></div><div><br></div><div>Is the string null terminated when there’s not enough room for the whole string? IMO it should be.</div><div><br></div><div>-Jim</div><div><br></div><div><br></div><div><br></div><div><div><div>On Jun 28, 2013, at 2:15 PM, Filip Pizlo <<a href="mailto:fpizlo@apple.com">fpizlo@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: 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;">Ping!<br><br>-F<br><br><br>On Jun 27, 2013, at 8:05 PM, Filip Pizlo <<a href="mailto:fpizlo@apple.com">fpizlo@apple.com</a>> wrote:<br><br><blockquote type="cite">The fact that this isn't already exposed makes using some other C APIs, like the disassembler API, really awkward in a JIT context - you don't want to have to describe to LLVM what the target name is for the process you're in, when LLVM already has this information.<br><br>The challenge here is that sys::getProcessTriple() returns a std::string that it allocates itself. So we can't just have a "LLVMGetProcessTarget()" that returns a char*, unless we then require the user to free() that char*. I considered that but I feared that it would have been confusing to users: for example LLVMGetTarget() returns a char* that doesn't require deallocation. So, to make the ownership rules more explicit, I made the API take an already allocated char* and a size.<br><br>I wanted to allow for this idiom for people who don't want to guess the right string size:<br><br>size_t targetSize = LLVMGetProcessTarget(0, 0);<br>char *target = malloc(targetSize);<br>LLVMGetProcessTarget(target, targetSize);<br><br>This means that I couldn't just call snprintf(). I actually ended up getting the buffer allocation wrong on the first try, so I include some test coverage to make sure that it really isn't messed up.<br><br>-Filip<br><br><GetProcessTarget.patch><br></blockquote><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</div></blockquote></div><br></div></body></html>