<div dir="ltr">Great, thanks, LGTM!</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Nov 26, 2013 at 7:32 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thank you for the suggestions -- the attached patch applies them.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Tue, Nov 26, 2013 at 9:57 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On Tue, Nov 26, 2013 at 6:34 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> On Tue, Nov 26, 2013 at 5:57 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> wrote:<br>
>> > On Tue, Nov 26, 2013 at 2:41 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Tue, Nov 26, 2013 at 5:35 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> >> wrote:<br>
>> >> > Looks great! Just a couple of questions:<br>
>> >> ><br>
>> >> > 1) In CalculateDiagnostic, it looks like you sometimes pick a<br>
>> >> > diagnostic<br>
>> >> > that's not an exact match for the subjects list. What cases do we hit<br>
>> >> > where<br>
>> >> > we don't have an exact match, and have you considered extending the<br>
>> >> > list<br>
>> >> > to<br>
>> >> > cover those too? (I also wonder whether we can do better here --<br>
>> >> > maybe<br>
>> >> > by<br>
>> >> > including Attr.td into DiagnosticSemaKinds.td and generating extra<br>
>> >> > diagnostics from Attr.td or similar -- but I don't want that blocking<br>
>> >> > this<br>
>> >> > patch!).<br>
>> >><br>
>> >> Both of those cases kind of bug me, but there were a substantial<br>
>> >> number of attributes using those values.  carries_dependency,<br>
>> >> no_*_analysis, check lock, etc. I intend to resolve that sort of thing<br>
>> >> on a case-by-case basis in future patches, hopefully resolving the<br>
>> >> inconsistencies. I avoided it here so I wouldn't muddy the patch or<br>
>> >> annoy DeLesley too much. ;-)<br>
>> ><br>
>> ><br>
>> > I would be happier if you moved over to switching on the currently-used<br>
>> > value combinations, so we can see explicitly in the code which sets of<br>
>> > subjects map to which diagnostics, and we can get a compile break if<br>
>> > someone<br>
>> > adds a new set which we weren't expecting and can't diagnose well.<br>
>><br>
>> This is using the currently-used diagnostics by emulating the existing<br>
>> behavior. Also, the patch already emits diagnostics if a new pattern<br>
>> comes up in Attr.td that we cannot handle.<br>
><br>
><br>
> Not exactly -- for instance, we claim "we can handle" any combination of<br>
> subjects that includes Func | ObjCMethod | Block, even though we can't<br>
> actually handle (for instance) Func | ObjCMethod | Block | Type in a<br>
> reasonable way. I'm suggesting you explicitly enumerate the sets of subjects<br>
> we can handle: replace the default case in your switch with:<br>
><br>
>   case Func | ObjCMethod | Block:<br>
>   case Func | ObjCMethod | Block | Var: // FIXME: 'sentinel' attribute needs<br>
> a better diagnostic<br>
>     return "ExpectedFunctionMethodOrBlock";<br>
>   case Func | ObjCMethod | Class: return "ExpectedFunctionMethodOrClass";<br>
>   // ...<br>
><br>
> ... and so on.<br>
><br>
>><br>
>> This was how I ensured I<br>
>> covered all of the cases.<br>
>><br>
>> Or are you asking me to comment out the Subjects in Attr.td for which<br>
>> the mapping is problematic, remove the mismatches from<br>
>> ClangAttrEmitter.cpp and go from there?<br>
>><br>
>> ><br>
>> >><br>
>> >> > 2) In GenerateAppertainsTo, you bail out for an attribute with custom<br>
>> >> > parsing. I find that surprising -- that flag affects how the<br>
>> >> > attribute<br>
>> >> > is<br>
>> >> > parsed into an AttributeList, but I don't see why it should have any<br>
>> >> > implications on subject checking. Checking SemaHandler here would<br>
>> >> > make<br>
>> >> > sense<br>
>> >> > to me, but that looks unnecessary. What's this for?<br>
>> >><br>
>> >> It may be unnecessary, but it's there because custom parsing<br>
>> >> attributes generally have more complex subjects anyway (the only one<br>
>> >> which kind of came close was type_tag_for_datatype, but the rest are<br>
>> >> complex). Also, anything that's custom parsed automatically opts out<br>
>> >> of common attribute handling as it stands. This was another area I was<br>
>> >> intending to do future research on.<br>
>> ><br>
>> ><br>
>> > I'd prefer to see the Subjects commented out for those attributes rather<br>
>> > than giving 'HasCustomParsing' additional meaning. Right now, it's<br>
>> > pretty<br>
>> > clearly documented as saying 'the parsing rules don't match Args', and<br>
>> > that's unrelated to what you're doing here.<br>
>><br>
>> That's easy enough.<br>
>><br>
>> ~Aaron<br>
><br>
><br>
</div></div></blockquote></div><br></div>