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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
