[PATCH] D112334: [clang-tidy] Suppress readability-static-access-through-instance for CUDA built-in variables

Mon Oct 25 09:06:45 PDT 2021

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