[PATCH] Generate diagnostic checks for attribute subjects

[Aaron Ballman]

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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AttrSubject.patch
Type: application/octet-stream
Size: 65771 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131126/84adaeae/attachment.obj>