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