On Thu, Dec 1, 2011 at 1:38 AM, Alexander Potapenko <[email protected]>wrote:
> On Thu, Dec 1, 2011 at 1:32 PM, Chandler Carruth <[email protected]> > wrote: > > On Thu, Dec 1, 2011 at 1:28 AM, Alexander Potapenko <[email protected]> > > wrote: > >> > >> Attached the new version, PTAL > >> > >> > + if (Args.hasArg(options::OPT_faddress_sanitizer)) { > >> > Please > >> > > >> > > use Args.hasFlag(options::OPT_faddress_sanitizer, > options::OPT_fno_address_sanitizer, > >> > false)) > >> Done > >> > > >> > and hide the flag checking inside addAsanRTDarwinExe/etc > >> Then this should be something like "maybeAddAsan...", because this > >> function is called unconditionally and other devs may think that we > >> always add ASan. > >> I've renamed the correspoding Linux function as well. > > > > > > I don't agree... The function is responsible for adding what Address > > Sanitizer needs, and it needs nothing if disabled. This matches several > > other 'addFoo' functions in the driver. > > Sounds convincing, I really haven't noticed other "addFoo" functions. > Fixed. > 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. =] 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
