<div dir="ltr">On Tue, Nov 26, 2013 at 2:41 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="im">On Tue, Nov 26, 2013 at 5:35 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> 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 where<br>
> we don't have an exact match, and have you considered extending the list to<br>
> cover those too? (I also wonder whether we can do better here -- maybe 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 this<br>
> patch!).<br>
<br>
</div>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. ;-)</blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> 2) In GenerateAppertainsTo, you bail out for an attribute with custom<br>
> parsing. I find that surprising -- that flag affects how the attribute 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 sense<br>
> to me, but that looks unnecessary. What's this for?<br>
<br>
</div>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.</blockquote><div><br></div><div>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. </div>
</div></div></div>