<div dir="ltr">You get a single email reply for the two of you. :)<div><br></div><div>Fixed the requests with the following:</div><div><br></div><div><div>dzur:~/sources/llvm/tools/clang> git svn dcommit </div><div>Committing to <a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_svn_llvm-2Dproject_cfe_trunk&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=thWxd50vV7WQ6aGrSpHhXRfIeX5tLMzKB_u8pDszk5Y&s=MNDmBoWFfEZeT3F5lVl0VW7FUSdXHW5t4WZeaBi-9VM&e=">https://llvm.org/svn/llvm-project/cfe/trunk</a> ...</div><div><span class="Apple-tab-span" style="white-space:pre">    </span>M<span class="Apple-tab-span" style="white-space:pre">   </span>include/clang/Basic/AttrDocs.td</div><div>Committed r241524</div><div><span class="Apple-tab-span" style="white-space:pre">      </span>M<span class="Apple-tab-span" style="white-space:pre">   </span>include/clang/Basic/AttrDocs.td</div><div>r241524 = 958aa2fcd7ce159211cb76409d97efdbef1d1524 (refs/remotes/origin/master)</div><div><span class="Apple-tab-span" style="white-space:pre">        </span>M<span class="Apple-tab-span" style="white-space:pre">   </span>lib/CodeGen/CGCall.cpp</div><div><span class="Apple-tab-span" style="white-space:pre">       </span>M<span class="Apple-tab-span" style="white-space:pre">   </span>test/CodeGen/attr-target.c</div><div>Committed r241525</div><div><span class="Apple-tab-span" style="white-space:pre">   </span>M<span class="Apple-tab-span" style="white-space:pre">   </span>test/CodeGen/attr-target.c</div><div><span class="Apple-tab-span" style="white-space:pre">   </span>M<span class="Apple-tab-span" style="white-space:pre">   </span>lib/CodeGen/CGCall.cpp</div><div>r241525 = 3b4cd0da85bd32acd27e4459f7e7e9f4658ce289 (refs/remotes/origin/master)</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>M<span class="Apple-tab-span" style="white-space:pre">   </span>include/clang/Basic/Attr.td</div><div><span class="Apple-tab-span" style="white-space:pre">  </span>M<span class="Apple-tab-span" style="white-space:pre">   </span>lib/CodeGen/CGCall.cpp</div><div>Committed r241526</div><div><span class="Apple-tab-span" style="white-space:pre">       </span>M<span class="Apple-tab-span" style="white-space:pre">   </span>lib/CodeGen/CGCall.cpp</div><div><span class="Apple-tab-span" style="white-space:pre">       </span>M<span class="Apple-tab-span" style="white-space:pre">   </span>include/clang/Basic/Attr.td</div><div>r241526 = 7bf902c36d7cbed7343d99c1c6737672d00fd462 (refs/remotes/origin/master)</div><div><br></div><div>A few comments:</div><div><br></div><div>a) Documentation - it's not good, but you're right, some is better than nothing. Full documentation of the various backend features sounds like something that might be handled in a different way, but I'm still thinking about it. Until then though, some documentation :)</div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> +      SubjectList<[Function], ErrorDiag, "ExpectedFunctionMethodOrClass">;<br>
<br>
The diagnostic for this does not match the subject list, and I don't<br>
understand why. It only applies to functions, but the diagnostic will<br>
tell the user it can apply to classes and Obj-C methods, which is not<br>
good.<br></blockquote><div><br></div><div>Uh, right. Oops and thanks.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +    if (FD) {<br>
> +      if (const TargetAttr *TD = FD->getAttr<TargetAttr>()) {<br>
<br>
Better to use const auto here instead of spelling the type twice.<br></blockquote><div><br></div><div>Sure. I'm more conservative with my use of auto, but I'm here anyhow.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +        StringRef FeaturesStr = TD->getFeatures();<br>
> +        SmallVector<StringRef, 1> AttrFeatures;<br>
> +        FeaturesStr.split(AttrFeatures, ",");<br>
<br>
Does the backend handle spaces and tabs sensibly? eg)<br>
[[gnu::target("foo, bar,              baz")]]?<br></blockquote><div><br></div><div>Nope. Neither did gcc. I've fixed it for us and it handles some weird whitespace. I don't handle arbitrary whitespace for arch= and tune= but if you think it's something someone could do I could start regexp matching them.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +static void handleTargetAttr(Sema &S, Decl *D, const AttributeList &Attr) {<br>
> +  // TODO: Validation should use a backend target library that specifies<br>
> +  // the allowable subtarget features and cpus. We could use something like a<br>
> +  // TargetCodeGenInfo hook here to do validation.<br>
<br>
Is there a rough timeline as to when this TODO will be fixed? I'm<br>
worried about attribute with weird, silent misbehavior if someone has<br>
a simple typo.<br>
<br></blockquote><div><br></div><div>Well, the backend would complain if it couldn't parse the attribute so I'm less worried about that - that said, I do plan on adding some verification of the backend features I know about based on the other work I did for the __builtin_cpu_supports work.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
This could be written without the extra declarations, but it's not<br>
that big of a deal either.<br>
<br></blockquote><div><br></div><div>Enh, I went ahead and left this. It feels more comforting to me :)</div><div><br></div><div>Thanks a lot for the review. Very appreciated :)</div><div><br></div><div>-eric</div><div> </div></div></div></div>