balazske added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:27 + : public Checker<check::PreStmt<ArraySubscriptExpr>> { + mutable std::unique_ptr<BugType> BT; + ---------------- The bug type can be initialized here: `BT{this, "...", "..."}` How did this compile with only one text argument to constructor? I think the first is a short name of the bug, second is a category that is not omittable. The text that is used here should be passed to the `BugReport`. `BugType::getDescription` returns the "name" of the bug that is the first argument. But I am not sure what matters from these texts, the code design looks confusing. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:50 + if (isa<IntegerLiteral>(Index->IgnoreParenCasts())) + return; + ---------------- `if (isa<IntegerLiteral>(Index))` should be used. `IntegerLiteral` is a subclass of `Expr`, not a `QualType`. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:72 + llvm::APSInt::getMaxValue(C.getASTContext().getIntWidth(IndexType), + IndexType->isUnsignedIntegerType()); + const nonloc::ConcreteInt MaxIndexValue{MaxIndex}; ---------------- I would use `SVB.getBasicValueFactory().getMaxValue(IndexType)` to get the max value. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:82 + SVB.evalBinOp(State, BO_Sub, SizeInElements, ConstantOne, + C.getASTContext().UnsignedLongLongTy); + ---------------- `SValBuilder::getArrayIndexType` should be better than the `UnsignedLongLongTy`. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:86 + const SVal TypeCanIndexEveryElement = SVB.evalBinOp( + State, BO_LE, SizeInElementsMinusOne, MaxIndexValue, IndexType); + ---------------- I think `SVB.getConditionType()` should be used for condition result. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:121 + // Mark the array expression interesting. + R->markInteresting(FirstElement->getSuperRegion()); + C.emitReport(std::move(R)); ---------------- I am not sure if this `markInteresting` call is correct. ================ Comment at: clang/test/Analysis/sufficient-size-array-indexing-32bit.c:37 +const short two_byte_signed_index = 1; // sizeof(short) == 2 +const int four_byte_signed_index = 1; // sizeof(int) == 4 + ---------------- I do not know if it is safe to make such assumptions about `sizeof`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69318/new/ https://reviews.llvm.org/D69318 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits