[PATCH] Generate diagnostic checks for attribute subjects

[Richard Smith]

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/66287b9c/attachment.html>