On Fri, Nov 30, 2012 at 3:01 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
<br>
================<br>
Comment at: llvm/tools/clang/include/clang/Basic/DiagnosticDriverKinds.td:104-107<br>
@@ -103,2 +103,6 @@<br>
</div><div class="im">   "AddressSanitizer on Android requires '-pie'">;<br>
</div><div class="im">+def err_drv_tsan_requires_pie : Error<<br>
+  "ThreadSanitizer requires '-pie'">;<br>
+def err_drv_msan_requires_pie : Error<<br>
+  "MemorySanitizer requires '-pie'">;<br>
 def err_drv_unknown_objc_runtime : Error<<br>
----------------<br>
</div><div class="im">Evgeniy Stepanov wrote:<br>
> Dmitri Gribenko wrote:<br>
> > Chandler Carruth wrote:<br>
> > > You don't need two diagnostics:<br>
> > ><br>
> > >   def err_drv_sanitizer_requires_pie : Error<<br>
> > >     "%select{thread|memory}0 sanitizer requires the '-pie' option">;<br>
> > I think it is better to use two diagnostics since %select would require magic numbers in the source.<br>
> I also prefer two diagnosticts.<br>
><br>
> Btw, %select{a|b}0 evaluates to "b" if true, and "a" if false. I find this _really_ weird.<br>
><br>
</div>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.<br>
<br>
I also dislike the duplicated text intensely.<br>
<br>
A good solution would be something like this:<br>
<br>
  def err_drv_sanitizer_requires_pie : Error<<br>
    "%select{thread|memory}0 sanitizer requires the '-pie' option">,<br>
    SelectNames<0, { SDK_Thread, SDK_Memory }>;<br>
<br>
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.<br>
<br>
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.</blockquote><div><br></div><div>We already have diag::err_drv_argument_only_allowed_with for indicating that one argument requires another.</div>
</div>