Adding Daniel, as this is mostly about libraries. <div><br></div><div>+    // LibAsan is "../lib/clang/linux/ArchName/libclang_rt.asan.a<br><div>s/linux/darwin/ (in comments)</div><div><br></div><div>--kcc </div><div>
<br></div><div><br></div><div><br><br><div class="gmail_quote">On Thu, Dec 1, 2011 at 1:44 AM, Alexander Potapenko <span dir="ltr"><<a href="mailto:glider@google.com">glider@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="HOEnZb"><div class="h5">On Thu, Dec 1, 2011 at 1:41 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
> On Thu, Dec 1, 2011 at 1:38 AM, Alexander Potapenko <<a href="mailto:glider@google.com">glider@google.com</a>><br>
> wrote:<br>
>><br>
>> On Thu, Dec 1, 2011 at 1:32 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>><br>
>> wrote:<br>
>> > On Thu, Dec 1, 2011 at 1:28 AM, Alexander Potapenko <<a href="mailto:glider@google.com">glider@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> Attached the new version, PTAL<br>
>> >><br>
>> >> > +  if (Args.hasArg(options::OPT_faddress_sanitizer)) {<br>
>> >> > Please<br>
>> >> ><br>
>> >> ><br>
>> >> > use Args.hasFlag(options::OPT_faddress_sanitizer, options::OPT_fno_address_sanitizer,<br>
>> >> > false))<br>
>> >> Done<br>
>> >> ><br>
>> >> > and hide the flag checking inside addAsanRTDarwinExe/etc<br>
>> >> Then this should be something like "maybeAddAsan...", because this<br>
>> >> function is called unconditionally and other devs may think that we<br>
>> >> always add ASan.<br>
>> >> I've renamed the correspoding Linux function as well.<br>
>> ><br>
>> ><br>
>> > I don't agree... The function is responsible for adding what Address<br>
>> > Sanitizer needs, and it needs nothing if disabled. This matches several<br>
>> > other 'addFoo' functions in the driver.<br>
>><br>
>> Sounds convincing, I really haven't noticed other "addFoo" functions.<br>
>> Fixed.<br>
><br>
> FWIW, this patch looks fine to me with one nit: please don't introduce more<br>
> trailing whitespace. It's important (for the annotation history) to not<br>
> strip trailing whitespace that already exists in the files, but I would ask<br>
> that you don't introduce more. =]<br>
> Still, I thing someone more familiar with Darwin should look at this before<br>
> it goes in... I just can't realistically review it for correctness on that<br>
> platform.<br>
<br>
</div></div>Ok, done.<br>
Waiting for someone with Darwin background to take a look.<br>
</blockquote></div><br></div></div>