<br><br><div class="gmail_quote">On Fri, Nov 30, 2012 at 12:01 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.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"><br>
<br>
================<br>
Comment at: llvm/tools/clang/include/clang/Basic/DiagnosticDriverKinds.td:104-107<br>
@@ -103,2 +103,6 @@<br>
"AddressSanitizer on Android requires '-pie'">;<br>
+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.<br>
<div class="im"><br></div></blockquote><div><br>Repurposing the thread (hope the new name and recipients will be enough).<br><br>While I (generally) like the idea of this enum, I do worry a bit: I think that in a number of cases a single enum is shared between multiple diagnostics. It's not a barrier to implementation, but something to account for => maybe this means that a new specific "class" should be specified in TableGen (giving both the type and enumerator list) and then linked in into the diagnostic.<br>
<br>I fail to see however how a "typed" approach could be used with the streaming interface used to fill in the diagnosis. Did I miss something ?<br><br>And in general, whilst the streaming "interface" of diagnostics is "easy", it's unfortunately error-prone. A more principled approach could be to generate a method for each diagnostic, and this method would have "proper" arguments. I am not sure this is tractable though, because of the huge number of diagnostics it might slow down compilation times considerably.<br>
<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
================<br>
Comment at: llvm/tools/clang/include/clang/Basic/DiagnosticDriverKinds.td:104-107<br>
@@ -103,2 +103,6 @@<br>
"AddressSanitizer on Android requires '-pie'">;<br>
+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>Chandler Carruth wrote:<br>
<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.<br>
The idea is to model an enumeration of selectors, not a bool... The value accepted by the diagnostic engine is technically an 'unsigned'.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D146" target="_blank">http://llvm-reviews.chandlerc.com/D146</a><br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br>