[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71707#1821903 <https://reviews.llvm.org/D71707#1821903>, @jankratochvil wrote:

> In D71707#1796671 <https://reviews.llvm.org/D71707#1796671>, @aaron.ballman wrote:
>
> > Have you considered language extensions like `__ptr32`, `__ptr64`, `__sptr`, and `__uptr` (https://docs.microsoft.com/en-us/cpp/cpp/ptr32-ptr64?view=vs-2019) which we also support?
>
>
> I think only `__uptr` support would matter to prevent some false positives. I am not sure it is used anywhere. Should I really implement it?


It's not super common so if it's a problem, you can punt on it. It is used within the Win32 SDK though (I found at least a few uses of it). If you don't want to add support for them right now, you can just throw a FIXME into the code so we remember to add support for it at some point.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp:32
+  for (;;) {
+    std::string str = CheckType.getAsString();
+    if (strset.count(str))
----------------
`str` should be `Str`.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp:46
+
+  static const auto intptrs = llvm::StringSet<>{"uintptr_t", "intptr_t"};
+  if (find_type(intptrs, DestType, Context))
----------------
`static llvm::StringSet<> IntPtrs{"uintptr_t", "intptr_t"};`


================
Comment at: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp:67
+
+  static const auto intptr = llvm::StringSet<>{"intptr_t"};
+  if (!find_type(intptr, SourceType, Context))
----------------
Similar here as above.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h:28
+private:
+  bool find_type(const llvm::StringSet<> &strset, QualType CheckType,
+                 const ASTContext &Context) const;
----------------
aaron.ballman wrote:
> These should be `findType`, `checkPointer`, and `checkIntegral` by our usual naming conventions. Also, `strset` should be `StrSet` per conventions (same in the definition).
This function doesn't need to be a member function, it doesn't rely on the class state whatsoever. I think the function should not take the container but instead be a templated function accepting iterators.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h:28-31
+  bool find_type(const llvm::StringSet<> &strset, QualType CheckType,
+                 const ASTContext &Context) const;
+  void check_pointer(const CastExpr *MatchedCast, const ASTContext &Context);
+  void check_integral(const CastExpr *MatchedCast, const ASTContext &Context);
----------------
These should be `findType`, `checkPointer`, and `checkIntegral` by our usual naming conventions. Also, `strset` should be `StrSet` per conventions (same in the definition).


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst:24
+
+The only supported cast from a pointer to integer is ``uintptr_t``.
+
----------------
This seems outdated?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp:17
+  t2 t = (t2)p;
+}
----------------
jankratochvil wrote:
> aaron.ballman wrote:
> > I'd also like to see tests for implicit casts as well as pointer width and sign extension language extensions.
> Done implicit cases.  But I do not understand what you mean by the rest - could you provide an example? Thanks.
I was talking about test cases involving `__ptr32` or `__uptr`, if we wanted to support them.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp:21
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  // uint64_t u64implicit = p; // error: cannot initialize a variable of type 'uint64_t' (aka 'unsigned long') with an lvalue of type 'void *' [clang-diagnostic-error]
+  uint64_t u64reinterpret = reinterpret_cast<uint64_t>(p);
----------------
Remove commented out code.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp:24
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  // uint64_t u64static = static_cast<uint64_t>(p); // error: static_cast from 'void *' to 'uint64_t' (aka 'unsigned long') is not allowed
+  uintptr_t up = (uintptr_t)p;
----------------
Remove commented out code.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp:32
+  t2 t = (t2)p;
+  intptr_t ip = reinterpret_cast<intptr_t>(p);
+  __uint128_t u64ipimplicit = ip;
----------------
Can you also show that C-style casts to `intptr_t` are not diagnosed? e.g., `intptr_t ip2 = (inptr_t)p;`


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp:37
+  // CHECK-MESSAGES: :[[@LINE-1]]:54: warning: do not use cast of pointer-like 'intptr_t' (aka 'long') to a wider type '__uint128_t' (aka 'unsigned __int128') which will sign-extend [bugprone-pointer-cast-widening]
+  // __uint128_t u64ipreinterpret = reinterpret_cast<__uint128_t>(ip); // error: reinterpret_cast from 'intptr_t' (aka 'long') to '__uint128_t' (aka 'unsigned __int128') is not allowed
+}
----------------
Remove commented-out code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71707/new/

https://reviews.llvm.org/D71707