[PATCH] Generate diagnostic checks for attribute subjects

[Aaron Ballman]

On Tue, Nov 26, 2013 at 5:57 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Tue, Nov 26, 2013 at 2:41 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Tue, Nov 26, 2013 at 5:35 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > Looks great! Just a couple of questions:
>> >
>> > 1) In CalculateDiagnostic, it looks like you sometimes pick a diagnostic
>> > that's not an exact match for the subjects list. What cases do we hit
>> > where
>> > we don't have an exact match, and have you considered extending the list
>> > to
>> > cover those too? (I also wonder whether we can do better here -- maybe
>> > by
>> > including Attr.td into DiagnosticSemaKinds.td and generating extra
>> > diagnostics from Attr.td or similar -- but I don't want that blocking
>> > this
>> > patch!).
>>
>> Both of those cases kind of bug me, but there were a substantial
>> number of attributes using those values.  carries_dependency,
>> no_*_analysis, check lock, etc. I intend to resolve that sort of thing
>> on a case-by-case basis in future patches, hopefully resolving the
>> inconsistencies. I avoided it here so I wouldn't muddy the patch or
>> annoy DeLesley too much. ;-)
>
>
> I would be happier if you moved over to switching on the currently-used
> value combinations, so we can see explicitly in the code which sets of
> subjects map to which diagnostics, and we can get a compile break if someone
> adds a new set which we weren't expecting and can't diagnose well.

This is using the currently-used diagnostics by emulating the existing
behavior. Also, the patch already emits diagnostics if a new pattern
comes up in Attr.td that we cannot handle. This was how I ensured I
covered all of the cases.

Or are you asking me to comment out the Subjects in Attr.td for which
the mapping is problematic, remove the mismatches from
ClangAttrEmitter.cpp and go from there?

>
>>
>> > 2) In GenerateAppertainsTo, you bail out for an attribute with custom
>> > parsing. I find that surprising -- that flag affects how the attribute
>> > is
>> > parsed into an AttributeList, but I don't see why it should have any
>> > implications on subject checking. Checking SemaHandler here would make
>> > sense
>> > to me, but that looks unnecessary. What's this for?
>>
>> It may be unnecessary, but it's there because custom parsing
>> attributes generally have more complex subjects anyway (the only one
>> which kind of came close was type_tag_for_datatype, but the rest are
>> complex). Also, anything that's custom parsed automatically opts out
>> of common attribute handling as it stands. This was another area I was
>> intending to do future research on.
>
>
> I'd prefer to see the Subjects commented out for those attributes rather
> than giving 'HasCustomParsing' additional meaning. Right now, it's pretty
> clearly documented as saying 'the parsing rules don't match Args', and
> that's unrelated to what you're doing here.

That's easy enough.

~Aaron