tra added inline comments.
================ Comment at: lib/Sema/SemaCUDA.cpp:99 + if (!D->hasAttrs()) + return false; + for (Attr *Attribute : D->getAttrs()) { ---------------- jlebar wrote: > Is this early return necessary? Yes. Otherwise D->getAttrs() will trigger assert(hasAttrs) if we don't have any attributes. ================ Comment at: lib/Sema/SemaCUDA.cpp:107 + } + return false; +} ---------------- jlebar wrote: > Could we write this function as > > return llvm::any_of(D->getAttrs(), [&](Attr *Attribute) { > return isa<A>(Attribute) && (!IgnoreImplicitAttr || > !Attribute->isImplicit()) }); Done. ================ Comment at: lib/Sema/SemaCUDA.cpp:896 + // needed to ensure that FD and its template have the same + // effective target. + if (CUDAGlobalAttr *Attr = OldFD->getAttr<CUDAGlobalAttr>()) { ---------------- jlebar wrote: > This comment is confusing because this function's implementation doesn't have > anything to do with templates, but the first sentence says we're copying from > a template to "FD". > > In addition to cleaning this up, maybe we should move the comment into Sema.h? I've changed second argument type to `const FunctionTemplateDecl*` to reflect my intent a bit better. Moved the comment to Sema.h. ================ Comment at: lib/Sema/SemaTemplate.cpp:7048 // target attributes into account, we perform target match check // here and reject candidates that have different target. if (LangOpts.CUDA && ---------------- jlebar wrote: > Missing some articles: > > Target attributes are part of the cuda function signature, so the deduced > template's cuda target must match XXX [1]. Given that regular template > deduction [2] does not take target attributes into account, we reject > candidates here that have a different target. > > [1] I am not sure what XXX should be. The deduced template's cuda target > must match what, exactly? > > [2] What is "regular template deduction"? [1] it must match the target of its template. [2] "C++ template deduction" Rephrased the comment based on IRL conversation w/ jlebar@. ================ Comment at: lib/Sema/SemaTemplate.cpp:7173 + if (LangOpts.CUDA) + inheritCUDATargetAttrs(FD, Specialization); + ---------------- jlebar wrote: > Isn't this backwards? The function is `to, from`? If this is a bug, can we > add a test to catch this? The arguments have different types now, one of them const&, so it should be unambiguous what's input and what's output. As for order of inputs/outputs, my mental model is `result=input` as in memcpy(dest, src). Style guide does not seem to say anything on order of input/output arguments. https://reviews.llvm.org/D25845 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits