ASDenysPetrov added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1692-1694
+ const bool IsOneDimensionalArray =
+ !isa<ConstantArrayType>(CAT->getElementType());
+ if (IsOneDimensionalArray) {
----------------
martong wrote:
> aaron.ballman wrote:
> >
> +1 for Aaron's suggestion, but then it would be really helpful to have an
> explanatory comment. E.g.:
> ```
> if (!isa<ConstantArrayType>(CAT->getElementType())) { // This is a one
> dimensional array.
> ```
I think that self-descriptive code is better than comments nearby. And it does
not affect anything in terms of performance.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696
+ const llvm::APSInt &Idx = CI->getValue();
+ const uint64_t I = static_cast<uint64_t>(Idx.getExtValue());
+ // Use `getZExtValue` because array extent can not be negative.
----------------
martong wrote:
> aaron.ballman wrote:
> >
> This `static_cast` seems to be dangerous to me, it might overflow. Can't we
> compare `Idx` directly to `Extent`? I see that `Idx` is an `APSint` and
> `Extent` is an `APInt`, but I think we should be able to handle the
> comparison on the APInt level b/c that takes care of the signedness. And the
> overflow situation should be handled as well properly with `APInt`, given
> from it's name "arbitrary precision int". In this sense I don't see why do we
> need `I` at all.
We can't get rid of `I` because we use it below anyway in `I >=
InitList->getNumInits()` and `InitList->getInit(I)`.
I couldn't find any appropriate function to compare without additional checking
for signedness or bit-width adjusting.
I'll try to improve this snippet.
================
Comment at: clang/test/Analysis/initialization.c:1
-// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze
-analyzer-checker=core.builtin,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze -analyzer-config
eagerly-assume=false
-analyzer-checker=core.uninitialized.Assign,debug.ExprInspection -verify %s
----------------
martong wrote:
> I don't see how this change is related. How could the tests work before
> having the `uninitialized.Assign` enabled before?
I've added `glob_invalid_index1` and `glob_invalid_index2` functions which were
not there before.
`core.uninitialized.Assign` produces warnings for them.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104285/new/
https://reviews.llvm.org/D104285
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
