<div dir="ltr">On Tue, Nov 26, 2013 at 6:34 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Nov 26, 2013 at 5:57 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> 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 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 list<br>
>> > to<br>
>> > cover those too? (I also wonder whether we can do better here -- 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 someone<br>
> adds a new set which we weren't expecting and can't diagnose well.<br>
<br>
</div></div>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.</blockquote><div><br></div><div>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:</div>
<div><br></div><div>  case Func | ObjCMethod | Block:</div><div>  case Func | ObjCMethod | Block | Var: // FIXME: 'sentinel' attribute needs a better diagnostic</div><div>    return "ExpectedFunctionMethodOrBlock";</div>
<div>  case Func | ObjCMethod | Class: return "ExpectedFunctionMethodOrClass";</div><div>  // ...</div><div><br></div><div>... and so on.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 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>
<div class="im"><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 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 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 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>
</div>That's easy enough.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div><br></div></div>