aaron.ballman added a comment.
In D112334#3084668 <https://reviews.llvm.org/D112334#3084668>, @carlosgalvezp
wrote:
> Thanks for the review!
>
> I'm not super happy with the commit in the sense that it feels more like a
> workaround than a proper fix, with the hardcoded name `__cuda_built_in` and
> so on. But honestly I don't know how implement it more properly, for example
> check if the `BaseTypeName` is a CUDA built-in type. Another issue is that
> currently one can't ignore warnings from system headers (like
> `__clang_cuda_builtin_vars.h`)if they come from a macro defined there. But I
> suppose that's a harder problem to tackle.
>
> But if you are happy with the patch I can go ahead and merge, and come back
> to it if I learn how to improve it later on as I get to learn more about the
> codebase.
I'm also a bit uneasy about it because I suspect this may apply to more than
just CUDA (e.g., I would imagine this applies to any builtin type). However,
there's no easy way to tell that these are builtin types because they're
exposed via a `Headers` include, and not via one of the intrinsic types in
`Types.h`. So I don't know of a better way to fix this and this seems like an
incremental improvement.
However, this won't be fool-proof either. A user could make a typedef to one of
these types; access through the typedef will still warn because of the
name-based matching. You could look through type sugar for that issue, but I'm
not certain how critical that is to solve.
================
Comment at:
clang-tools-extra/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:75
+ // Do not warn for CUDA built-in variables.
+ if (StringRef(BaseTypeName).contains("__cuda_builtin_"))
+ return;
----------------
carlosgalvezp wrote:
> aaron.ballman wrote:
> > I presume we want this to strictly be a prefix?
> Sure, but then I can't put the test in a named namespace. I changed it to
> anonymous namespace + link to Bugzilla, hope it works.
These types are not defined within a namespace in
`__clang_cuda_builtin_vars.h`, so I wouldn't expect them to be in the test file
TBH.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112334/new/
https://reviews.llvm.org/D112334
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
