[PATCH] Generate diagnostic checks for attribute subjects

[Richard Smith]

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.


> > 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131126/825a1446/attachment.html>