jlebar added a comment.
Doesn't look like we changed any testcases when we changed this behavior? The
change in behavior may be unobservable, but even still it seems worthwhile to
have tests that check that we're not doing any of the alternatives we
considered.
================
Comment at: lib/Sema/SemaCUDA.cpp:99
+ if (!D->hasAttrs())
+ return false;
+ for (Attr *Attribute : D->getAttrs()) {
----------------
Is this early return necessary?
================
Comment at: lib/Sema/SemaCUDA.cpp:107
+ }
+ return false;
+}
----------------
Could we write this function as
return llvm::any_of(D->getAttrs(), [&](Attr *Attribute) {
return isa<A>(Attribute) && (!IgnoreImplicitAttr ||
!Attribute->isImplicit()) });
================
Comment at: lib/Sema/SemaCUDA.cpp:893
+
+void Sema::inheritCUDATargetAttrs(FunctionDecl *NewFD, FunctionDecl *OldFD) {
+ // Propagate CUDA target attributes from template to FD. This is
----------------
Perhaps `ToFD`, `FromFD` would be better names than "new" and "old".
================
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>()) {
----------------
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?
================
Comment at: lib/Sema/SemaCUDA.cpp:912
+ }
+ }
+}
----------------
Could this be written as
template<typename AttrTy>
static void copyAttrIfPresent(FunctionDecl *NewFD, FunctionDecl *OldFD) {
if (A *Attribute = OldFD->getAttr<A>()) {
...
}
}
void Sema::inheritCUDATargetAttrs(...) {
copyAttrIfPresent<CUDAGlobalAttr>(NewFD, OldFD);
...
}
? In particular I am not sure why we need this particular control flow in this
function.
================
Comment at: lib/Sema/SemaDecl.cpp:8372
+ // may end up with different effective targets. Instead,
+ // specializations inherit target attributes from template in
+ // CheckFunctionTemplateSpecialization() call below.
----------------
"from their templates" "in *the* foo() call below".
Although because "from their template__s__" is kind of confusing, I might
rewrite to be singular:
Instead, a specialization inherits its target attributes from its template ...
================
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 &&
----------------
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"?
================
Comment at: lib/Sema/SemaTemplate.cpp:7171
+ // must inherit in order to have the same effective target as its
+ // template.
+ if (LangOpts.CUDA)
----------------
Suggest
A function template specialization inherits the target attributes of its
template. (We require the attributes explicitly in the code to match, but a
template may have implicit attributes by virtue e.g. of being constexpr, and it
passes these implicit attributes on to its specializations.)
================
Comment at: lib/Sema/SemaTemplate.cpp:7173
+ if (LangOpts.CUDA)
+ inheritCUDATargetAttrs(FD, Specialization);
+
----------------
Isn't this backwards? The function is `to, from`? If this is a bug, can we
add a test to catch this?
https://reviews.llvm.org/D25845
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
