[cfe-dev] Specifying enums in TableGen for uses of %select

[Matthieu Monrocq]

On Fri, Nov 30, 2012 at 12:01 PM, Chandler Carruth <chandlerc at gmail.com>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.
>
>
Repurposing the thread (hope the new name and recipients will be enough).

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.

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 ?

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.



> ================
> 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<
> ----------------
> Chandler Carruth wrote:
> > 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.
> The idea is to model an enumeration of selectors, not a bool... The value
> accepted by the diagnostic engine is technically an 'unsigned'.
>
>
> http://llvm-reviews.chandlerc.com/D146
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121130/7cbb7c79/attachment.html>