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
