lebedev.ri added a comment. Thank you for taking a look!
In D93822#2474236 <https://reviews.llvm.org/D93822#2474236>, @dblaikie wrote: > Haven't reviewed the code in detail, but the idea seems sound. That's reassuring! > Is there any prior art in other compilers you can draw data/experiences from? As far as i can tell, no other compiler diagnoses these cases, however e.g. GitHub's CodeQL static analysis tool complains about this by default: https://github.com/darktable-org/rawspeed/security/code-scanning/7?query=342efe49c0bef4bf2450051586d899f9d303ae62 > & having some data from some reasonably sized codebases on false positive > (how often the correct thing is to suppress the warning, versus fixing the > warning) > would probably be useful. I don't have those numbers presently (other than the long long diffs of fixes for true-positives i have already linked), but as a rule of thumb, iff a particular project in question never allocates more than `UINT_MAX` bytes / `UINT_MAX / sizeof(element)` elements, *all* issued diagnostics for said project *will* be false-positives by definition. So YMMV. > I would guess it doesn't meet the on-by-default bar we would prefer for Clang, Yeah, based on my initial observations, it's pretty chatty. > but might still be OK. I'm open to input here, but it would be good to have this at least in `-Wextra` (`-Weverything` always being a last resort option). ================ Comment at: clang/docs/ReleaseNotes.rst:64-74 + - ``-Wimplicit-widening-of-pointer-offset``: + + .. code-block:: c++ + + char* ptr_add(char *base, int a, int b) { + return base + a * b; // expected-warning {{result of multiplication in type 'int' is used as a pointer offset after an implicit widening conversion to type 'ssize_t'}} + } ---------------- dblaikie wrote: > Does this only trigger when the `sizeof(char*) > sizeof(int)`? (judging by > the test coverage I think that's the case) > > (ultimately, might be worth committing the two diagnostics separately - usual > sort of reasons, separation of concerns, etc) > Does this only trigger when the sizeof(char*) > sizeof(int)? (judging by the > test coverage I think that's the case) That should be the case, since `sizeof(size_t) == sizeof(char*)` and `// RUN: %clang_cc1 -triple i686-linux-gnu -fsyntax-only -Wimplicit-widening-of-pointer-offset -verify=silent %s` > (ultimately, might be worth committing the two diagnostics separately - usual > sort of reasons, separation of concerns, etc) Should i split this into two reviews to begin with? ================ Comment at: clang/test/Sema/implicit-widening-of-pointer-offset-in-array-subscript-expression.c:24 +void t1(char *base, int a, int b) { + // FIXME: test `[a * b]base` pattern? +} ---------------- dblaikie wrote: > Question is unclear - is there uncertainty about whether this should be > tested? Or is this a false negative case? In which case I'd probably include > the test showing no diagnostic and mention it could be fixed/improved? I may be misremembering things, but IIRC `a[b]` and `b[a]` is the same thing, but i'm not sure how to exercise the second spelling. I.e. i'm just not sure how to write a test for it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93822/new/ https://reviews.llvm.org/D93822 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
