[PATCH] Generate diagnostic checks for attribute subjects

[Richard Smith]

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