<div dir="ltr">On Wed, Sep 11, 2013 at 11:57 AM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">On Wed, Sep 11, 2013 at 1:32 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>

> SemaDeclAttr:2450 is underindented<br>
<br>
That appears to be existing code which was hyperindented, but I've<br>
changed the surrounding code.<br></blockquote><div><br></div><div>Err, sorry, I meant 2340:</div><div><br></div><div><div>+  // Complain about attempts to use protected visibility on targets</div><div>+  // (like Darwin) that don't support it.</div>
<div>+  if (type == VisibilityAttr::Protected &&</div><div>+    !S.Context.getTargetInfo().hasProtectedVisibility()) {</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

> ClangAttrEmitter:988 is underindented<br>
<br>
Good catch!<br>
<div class="im"><br>
><br>
> The writeConversion function you generate has undefined behavior if the<br>
> string is not a member of the enumeration (because -1 is not a representable<br>
> value in the enumeration type). How about using a<br>
> StringSwitch<Optional<EnumType> > ? (You could even return<br>
> Optional<EnumType> directly from ConvertStrToEnumType.)<br>
<br>
</div>Also a good catch!  I tried returning an Optional<EnumType>, but it<br>
made the surrounding code a bit more gnarly to work with.  So I left<br>
the signature the same, but changed the implementation.<br></blockquote><div><br></div><div>The 'if' statement in the generated code is overindented ;)</div><div><br></div><div>Otherwise, LGTM. Thanks!</div><div>
 </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Thanks for the review!<br>
<span class=""><font color="#888888"><br>
~Aaron<br>
</font></span><div class=""><div class="h5">><br>
><br>
> On Tue, Sep 10, 2013 at 7:44 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> Ping<br>
>><br>
>> On Fri, Sep 6, 2013 at 2:31 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> wrote:<br>
>> > This patch causes tablegen to generate a StringSwitch for attributes<br>
>> > that contain enum args, mapping from the enumerant string to the<br>
>> > proper enumeration value.  It resolves inconsistencies between<br>
>> > diagnostics, and brings us one step closer to more generalized error<br>
>> > reporting for attributes.<br>
>> ><br>
>> > ~Aaron<br>
><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>