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

Dmitri Gribenko gribozavr at gmail.com
Fri Nov 30 11:16:35 PST 2012


On Fri, Nov 30, 2012 at 8:42 PM, Matthieu Monrocq
<matthieu.monrocq at gmail.com> wrote:
> 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 ?

Unfortunately, I don't know what particular implementation Chandler
had in mind, but here's something that we can do: we could assign a
unique value for each enumerator across all such enumerations and do
the appropriate bounds check in runtime.

> 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.

And given that our diagnostics can essentially have a variable number
of arguments, but the number of arguments depends on some other
arguments' values ("%select{|%1}0"), this is even more challenging to
make completely safe.

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/



More information about the cfe-dev mailing list