danielmarjamaki marked 4 inline comments as done.
================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:41
@@ +40,3 @@
+ const Stmt *Parent = PM.getParent(Cast);
+ if (!Parent)
+ return;
----------------
a.sidorin wrote:
> Parent should always exist for an implicit cast. May be it's better to assert
> here?
If I comment out lines 41 and 42 and run "make check-all" I get:
FAIL: Clang :: Analysis/misc-ps-region-store.cpp (668 of 24574)
******************** TEST 'Clang :: Analysis/misc-ps-region-store.cpp'
FAILED ********************
....
#0 ...
#1 ...
...
#9 0x0000000002f1d062 llvm::isa_impl_cl<clang::BinaryOperator, clang::Stmt
const*>::doit(clang::Stmt const*)
/home/danielm/llvm/include/llvm/Support/Casting.h:96:0
================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:44
@@ +43,3 @@
+
+ const BinaryOperator *B = dyn_cast<BinaryOperator>(Parent);
+ if (!B)
----------------
a.sidorin wrote:
> Note that InitExprs of DeclStmts are not binary operators. So you will not
> get a warning on initialization and this test:
>
> ```
> void test(unsigned int p) {
> unsigned X = 1000;
> unsigned char uc = X; // expected-warning {{Loss of precision}}
> }
> ```
> will fail.
I have limited this patch hoping that it will be easier to triage and review.
Right now I only warn if there is assignment and RHS is a DeclRefExpr and only
for loss of precision. I will definitely look into initializations also later.
================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:49
@@ +48,3 @@
+ BinaryOperator::Opcode Opc = B->getOpcode();
+ if (Opc == BO_Assign || Opc == BO_MulAssign)
+ diagnoseLossOfPrecision(Cast, C);
----------------
a.sidorin wrote:
> It's not evident why do you omit other Assign operators here, like
> BO_SubAssign, BO_AddAssign and BO_DivAssign. As I see from your test, there
> are some problems with them. Could you add a comment?
DivAssign is obvious; if the rhs is too large there won't be loss of precision.
But I fail to think of additional problems to handle BO_AddAssign and
BO_SubAssign also immediately.. I'll add it in next patch
================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:158
@@ +157,3 @@
+
+ unsigned long long MaxVal = 1ULL << W;
+ if (canBeGreaterEqual(C, Cast->getSubExpr(), MaxVal))
----------------
NoQ wrote:
> `BasicValueFactory::getMaxValue(QualType)` might be useful.
Imho this seems harder.
To start with the getMaxValue returns 127 for a signed 8-bit integer.
I don't want to warn here:
S8 = 0xff; // <- no loss of precision, all bits are saved
So instead of 1 line I'd get 4+ lines:
SValBuilder &Bldr = C.getSValBuilder();
BasicValueFactory &BVF = Bldr.getBasicValueFactory();
QualType U = ResultType... // I don't know how to convert ResultType
const llvm::APSInt &MaxVal1 = BVF.getMaxValue(U);
Maybe I am missing something. Let me know if I do.
http://reviews.llvm.org/D13126
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
