<div class="gmail_quote">On Thu, Dec 1, 2011 at 1:38 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="im">On Thu, Dec 1, 2011 at 1:32 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> 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>
>> > 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>
</div>Sounds convincing, I really haven't noticed other "addFoo" functions.<br>
Fixed.<br>
</blockquote></div><br><div>FWIW, this patch looks fine to me with one nit: please don't introduce more trailing whitespace. It's important (for the annotation history) to not strip trailing whitespace that already exists in the files, but I would ask that you don't introduce more. =]</div>
<div><br></div><div>Still, I thing someone more familiar with Darwin should look at this before it goes in... I just can't realistically review it for correctness on that platform.</div>