aaron.ballman added a comment. Herald added a subscriber: whisperity. In D71707#1791394 <https://reviews.llvm.org/D71707#1791394>, @jankratochvil wrote:
> In D71707#1791280 <https://reviews.llvm.org/D71707#1791280>, @labath wrote: > > > - disallowing casts to intptr_t seems too restrictive -- I doubt many > > people are doing that, but I guess this type exists for a reason, and since > > the type (and it's signedness) is spelled out in the source, it shouldn't > > be too surprising that sign-extension can happen later > > > I was trying to find what is `intptr_t` good for and I haven't found any > valid reason. It seems to me nobody knows that either. Which is why I find > correct to report it. This checker has many false positives (or "not really a > bug") anyway. I agree that restricting casts to `intptr_t` is too restrictive. `intptr_t` is good for all the same things as `uintptr_t` -- it depends on why you want to use the pointer value as an integer value as to what it is good for. I'm also concerned about the number of false positives found by this checker, but I think fixing this would fix some of the more egregious false positives. >> - requiring a literal uintptr_t (or a typedef of it) may be also problematic >> -- the user could obtain an integer type of the same bit width through some >> other means (e.g. `#ifdef`). OTOH, without that (and just checking the bit >> width for instance), one would have to actually compile for a 32-bit target >> to get this warning. I don't know what's the practice for this in >> clang-tidy... > > Yes, I wanted first to check the widths but then I realized user would need a > 32-bit host for that which is too difficult to (1) get nowadays and (2) > primarily to build there LLVM. 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 those probably should factor into this new check as well. ================ Comment at: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp:21-22 +void PointerCastWideningCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus && !getLangOpts().C99) + return; + ---------------- Why limiting this to C++ and >= C99? You can get pointer widening casts through extensions in C89, for instance. ================ Comment at: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp:24 + + Finder->addMatcher(castExpr(unless(isInTemplateInstantiation())).bind("cast"), + this); ---------------- I think you should use `hasCastKind()` in this matcher as well. ================ Comment at: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp:36 + for (QualType DestTypeCheck = DestType;;) { + if (DestTypeCheck.getAsString() == "uintptr_t") + return; ---------------- How well does this work with something like `std::uinptr_t` from `<cstdint>`? ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp:3 + +#include <stdint.h> + ---------------- You should replicate the contents of the system header rather than including it for tests -- this ensures you are testing the same thing on all platforms. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp:11 + // CHECK-MESSAGES: :[[@LINE-1]]:19: 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] + intptr_t ip = reinterpret_cast<intptr_t>(p); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'intptr_t' (aka 'long') which may sign-extend [bugprone-pointer-cast-widening] ---------------- This should not diagnose. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp:17 + t2 t = (t2)p; +} ---------------- I'd also like to see tests for implicit casts as well as pointer width and sign extension language extensions. 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
