jlebar added inline comments.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:5628
   case AttributeList::AT_CUDAHost:
-    handleSimpleAttributeWithExclusions<CUDAHostAttr, CUDAGlobalAttr>(S, D,
-                                                                      Attr);
+    if (!D->hasAttr<CUDAHostAttr>())
+      handleSimpleAttributeWithExclusions<CUDAHostAttr, CUDAGlobalAttr>(S, D,
----------------
tra wrote:
> jlebar wrote:
> > Is this a functional change in this patch?  We don't check for duplicates 
> > of most other attributes, so I'm not sure why it should matter with these.  
> > If it does matter, we should have comments...
> This function copies over attributes from template to instantiation, so when 
> we already had explicit attributes we would end up with another one which has 
> potential to mess with our attempt to ignore implicit attributes. getAttr() 
> would return the first attribute it finds and if it happens to be implicit 
> one, explicit one would be ignored.
> 
> Plus, it was rather weird to see duplicate attributes hanging off 
> FunctionDecls in AST.
> This function copies over attributes from template to instantiation

It does?  I thought this just handled seeing an attribute in the source?

I am all for having a pretty AST, but

 * if the user explicitly puts two `__host__` attributes on the function, 
clearly we don't care about having the attribute twice on the function, and 
alternatively
 * if the user explicitly puts a `__host__` attribute on the function and we 
also somehow got an implicit attr there, this check seems wrong, since it will 
let the implicit one win over the explicit one?


================
Comment at: lib/Sema/SemaTemplate.cpp:7046
       if (LangOpts.CUDA &&
-          IdentifyCUDATarget(Specialization) != IdentifyCUDATarget(FD)) {
+          IdentifyCUDATarget(Specialization, true) !=
+              IdentifyCUDATarget(FD, true)) {
----------------
jlebar wrote:
> http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html
>   :)
> 
> At least please do
> 
>   IdentifyCUDATarget(Specialization, /* IgnoreImplicitTargetAttrs = */ true);
Please put the equals sign.  Without it, who knows whether the following call 
is sync or async:

  make_call(/* async */ false);

Also using the equals sign lets a clang-tidy check ensure that we're using the 
right name.


================
Comment at: lib/Sema/SemaTemplate.cpp:8119
     if (LangOpts.CUDA &&
-        IdentifyCUDATarget(Specialization) != IdentifyCUDATarget(Attr)) {
+        IdentifyCUDATarget(Specialization, true) != IdentifyCUDATarget(Attr)) {
       FailedCandidates.addCandidate().set(
----------------
jlebar wrote:
> Same here wrt bool param.
Same here.


https://reviews.llvm.org/D25845



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to