[PATCH] Generate diagnostic checks for attribute subjects

[Aaron Ballman]

Thanks! Committed in r195841

~Aaron

On Tue, Nov 26, 2013 at 10:40 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> Great, thanks, LGTM!
>
>
> On Tue, Nov 26, 2013 at 7:32 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> Thank you for the suggestions -- the attached patch applies them.
>>
>> ~Aaron
>>
>> On Tue, Nov 26, 2013 at 9:57 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > On Tue, Nov 26, 2013 at 6:34 PM, Aaron Ballman <aaron at aaronballman.com>
>> > wrote:
>> >>
>> >> 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.
>> >
>> >
>> > Not exactly -- for instance, we claim "we can handle" any combination of
>> > subjects that includes Func | ObjCMethod | Block, even though we can't
>> > actually handle (for instance) Func | ObjCMethod | Block | Type in a
>> > reasonable way. I'm suggesting you explicitly enumerate the sets of
>> > subjects
>> > we can handle: replace the default case in your switch with:
>> >
>> >   case Func | ObjCMethod | Block:
>> >   case Func | ObjCMethod | Block | Var: // FIXME: 'sentinel' attribute
>> > needs
>> > a better diagnostic
>> >     return "ExpectedFunctionMethodOrBlock";
>> >   case Func | ObjCMethod | Class: return
>> > "ExpectedFunctionMethodOrClass";
>> >   // ...
>> >
>> > ... and so on.
>> >
>> >>
>> >> 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
>> >
>> >
>
>