<div dir="ltr">Hi Tom,<div><br></div><div>Thanks for the changes. A couple comments:</div><div><br></div><div>1. It would be cleaner to move the initialized variable inside lto_initialize, and call it unconditionally in each LTO interface function. Also, it should probably be a static  function (unless we add it to the public interface in lto.h, but I don't see a need to do that now).</div>
<div><br></div><div>2. > +  // initialize the configured targets</div><div>Initial caps and punctuation in comments please.</div><div><br></div><div>Otherwise LGTM, thanks!</div><div><br></div><div> - Daniel</div></div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Sep 23, 2013 at 5:09 PM, Tom Roeder <span dir="ltr"><<a href="mailto:tmroeder@google.com" target="_blank">tmroeder@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Mon, Sep 23, 2013 at 4:30 PM, Peter Collingbourne <<a href="mailto:peter@pcc.me.uk">peter@pcc.me.uk</a>> wrote:<br>
> On Mon, Sep 23, 2013 at 04:06:50PM -0700, Tom Roeder wrote:<br>
</div><div class="im">>> Please take a look.<br>
><br>
> Hi Tom,<br>
><br>
>>  /**<br>
>> - * Initializes LLVM disassemblers.<br>
>> - * FIXME: This doesn't really belong here.<br>
>> + * Initializes LLVM Targets for lto. This must be called before<br>
>> + * creating an lto_code_gen_t or lto_module_t.<br>
>>   */<br>
>>  extern void<br>
>> -lto_initialize_disassembler(void);<br>
>> +lto_initialize(void);<br>
><br>
> I don't think this is right.  libLTO is a stable external interface used<br>
> by Apple's linker.  Can you make the lto_codegen_create and lto_module_create*<br>
> functions do the target initialization if needed?<br>
><br>
> Thanks,<br>
> --<br>
> Peter<br>
<br>
</div>Ah, thanks, I didn't know that.<br>
<br>
I've restored the LTODisassembler and its associated call in<br>
llvm-c/lto.h, and I've added a static variable "initialized" to<br>
capture the initialization state of the lto library as well as adding<br>
initialization calls in the module/code_gen creation paths. It feels a<br>
little kludgy, but the tools/lto library has already bought the notion<br>
of static state with the variable sLastErrorString, and I don't see<br>
another way of keeping this state short of changing the interface.<br>
According to the headers, it's legit to call the InitializeAll*<br>
functions multiple times, but I don't think we want to have to do it<br>
for every module handled by libLTO.so.<br>
<br>
Here's the changed version.<br>
<br>
Thanks,<br>
<br>
Tom<br>
</blockquote></div><br></div>