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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
