Done and committed. I also noticed that we do not catch conflicts between memory and other sanitizers.
Let me know if you are not comfortable with a bit of code duplication there. I think that N*(N-1) explicit checks (for N=3) is still better and more readable than generic code for arbitrary number of sanitizers. On Wed, Dec 5, 2012 at 2:35 AM, Richard Smith <[email protected]> wrote: > On Tue, Dec 4, 2012 at 3:04 AM, Evgeniy Stepanov <[email protected]>wrote: > >> Nice. Thanks for the pointer. >> Does the attached patch look good? >> > > Looks fine. Presumably a test needs updating too? > > >> On Tue, Dec 4, 2012 at 2:00 AM, Richard Smith <[email protected]>wrote: >> >>> On Fri, Nov 30, 2012 at 3:01 AM, Chandler Carruth >>> <[email protected]>wrote: >>> >>>> >>>> >>>> ================ >>>> Comment at: >>>> llvm/tools/clang/include/clang/Basic/DiagnosticDriverKinds.td:104-107 >>>> @@ -103,2 +103,6 @@ >>>> "AddressSanitizer on Android requires '-pie'">; >>>> +def err_drv_tsan_requires_pie : Error< >>>> + "ThreadSanitizer requires '-pie'">; >>>> +def err_drv_msan_requires_pie : Error< >>>> + "MemorySanitizer requires '-pie'">; >>>> def err_drv_unknown_objc_runtime : Error< >>>> ---------------- >>>> Evgeniy Stepanov wrote: >>>> > Dmitri Gribenko wrote: >>>> > > Chandler Carruth wrote: >>>> > > > You don't need two diagnostics: >>>> > > > >>>> > > > def err_drv_sanitizer_requires_pie : Error< >>>> > > > "%select{thread|memory}0 sanitizer requires the '-pie' >>>> option">; >>>> > > I think it is better to use two diagnostics since %select would >>>> require magic numbers in the source. >>>> > I also prefer two diagnosticts. >>>> > >>>> > Btw, %select{a|b}0 evaluates to "b" if true, and "a" if false. I find >>>> this _really_ weird. >>>> > >>>> I don't like that pattern any more than you do, but Clang's diagnostics >>>> use it *extensively*, adding a comment next to the magic number indicating >>>> which is which. >>>> >>>> I also dislike the duplicated text intensely. >>>> >>>> A good solution would be something like this: >>>> >>>> def err_drv_sanitizer_requires_pie : Error< >>>> "%select{thread|memory}0 sanitizer requires the '-pie' option">, >>>> SelectNames<0, { SDK_Thread, SDK_Memory }>; >>>> >>>> Where this automatically teaches the diagnostic engine that select #0 >>>> should accept a specific enum rather than an unsigned, and defines that >>>> enum in the diag namespace with the given enumerators. >>>> >>>> But this would be a huge (good) change and shouldn't be in this patch. >>>> I think I mildly prefer consistency with other diagnostics until we get >>>> something like this. >>> >>> >>> We already have diag::err_drv_argument_only_allowed_with for indicating >>> that one argument requires another. >>> >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
