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
