You get a single email reply for the two of you. :) Fixed the requests with the following:
dzur:~/sources/llvm/tools/clang> git svn dcommit Committing to https://llvm.org/svn/llvm-project/cfe/trunk ... M include/clang/Basic/AttrDocs.td Committed r241524 M include/clang/Basic/AttrDocs.td r241524 = 958aa2fcd7ce159211cb76409d97efdbef1d1524 (refs/remotes/origin/master) M lib/CodeGen/CGCall.cpp M test/CodeGen/attr-target.c Committed r241525 M test/CodeGen/attr-target.c M lib/CodeGen/CGCall.cpp r241525 = 3b4cd0da85bd32acd27e4459f7e7e9f4658ce289 (refs/remotes/origin/master) M include/clang/Basic/Attr.td M lib/CodeGen/CGCall.cpp Committed r241526 M lib/CodeGen/CGCall.cpp M include/clang/Basic/Attr.td r241526 = 7bf902c36d7cbed7343d99c1c6737672d00fd462 (refs/remotes/origin/master) A few comments: 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 :) > > > + SubjectList<[Function], ErrorDiag, > "ExpectedFunctionMethodOrClass">; > > The diagnostic for this does not match the subject list, and I don't > understand why. It only applies to functions, but the diagnostic will > tell the user it can apply to classes and Obj-C methods, which is not > good. > Uh, right. Oops and thanks. > + if (FD) { > > + if (const TargetAttr *TD = FD->getAttr<TargetAttr>()) { > > Better to use const auto here instead of spelling the type twice. > Sure. I'm more conservative with my use of auto, but I'm here anyhow. > > > + StringRef FeaturesStr = TD->getFeatures(); > > + SmallVector<StringRef, 1> AttrFeatures; > > + FeaturesStr.split(AttrFeatures, ","); > > Does the backend handle spaces and tabs sensibly? eg) > [[gnu::target("foo, bar, baz")]]? > 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. > > +static void handleTargetAttr(Sema &S, Decl *D, const AttributeList > &Attr) { > > + // TODO: Validation should use a backend target library that specifies > > + // the allowable subtarget features and cpus. We could use something > like a > > + // TargetCodeGenInfo hook here to do validation. > > Is there a rough timeline as to when this TODO will be fixed? I'm > worried about attribute with weird, silent misbehavior if someone has > a simple typo. > > 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. > > > This could be written without the extra declarations, but it's not > that big of a deal either. > > Enh, I went ahead and left this. It feels more comforting to me :) Thanks a lot for the review. Very appreciated :) -eric
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits