NoQ added a comment.
George's style comments are usually super spot-on. Please feel free to improve
my code. Also it was only written as a proof-of-concept because i failed to
explain my approach with natural language, so it definitely needs polishing.
I'd let you know when i disagree with anything :)
================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:472
+ if (BinaryOperator::isComparisonOp(Op) &&
+ Opts.shouldAggressivelySimplifyRelationalComparison()) {
+ if (ResultTy != SVB.getConditionType())
----------------
george.karpenkov wrote:
> I am confused why the option `shouldAggressivelySimplifyRelationalComparison`
> is checked here. Do we rewrite cases where `Op` is additive even if the
> option is not checked? It's generally better to check the flag outside of the
> rearranging function, so that the high-level logic can be seen.
> Do we rewrite cases where `Op` is additive even if the option is not checked?
Yes. Because for additive ops the presumably-expensive clamped-by-max/4 check
is not necessary (and is indeed skipped below).
This should be documented though.
================
Comment at: test/Analysis/svalbuilder-rearrange-comparisons.c:19
+ if (x < -(((int)INT_MAX) / 4))
+ exit(1);
+ return x;
----------------
george.karpenkov wrote:
> assert would express intent better
Yeah, but it's also a bit more painful to write in tests (i.e.
`__builtin_assert()` and macros).
================
Comment at: test/Analysis/svalbuilder-rearrange-comparisons.c:28
+ clang_analyzer_dump(x == y);
+ // expected-warning@-1{{((conj_$2{int}) - (conj_$5{int})) == 0}}
+}
----------------
george.karpenkov wrote:
> Can we also have integration-like tests where rearrangement would affect
> observable behavior?
>
> Also, `clang_analyzer_dump` is a debugging function, and I am not sure
> whether we should rely on its output in tests.
Yeah, this test would be painful to maintain if we change the dump output. I
guess we definitely need more integration tests.
I still wish there were ways to write "this" sort of tests without relying on
dumps. Eg.:
```
clang_analyzer_denote(x, "$x");
clang_analyzer_denote(y, "$y");
clang_analyzer_express(x == y); // expected-warning{{($x - $y) == 0}}
```
It should be easy to make an `SValVisitor` that does that.
Given how painful this would be to actually update these numerous tests, i'm
fine with leaving a comment explaining the above, as a bit of technical debt -
for anyone who would need to update them in the future.
We still need more integration tests though.
================
Comment at: test/Analysis/svalbuilder-rearrange-comparisons.c:100
+ clang_analyzer_dump(x == y);
+ // expected-warning@-1{{1 S32b}}
+}
----------------
This one, for instance, is essentially an integration test. You can easily
replace the last dump with clang_analyzer_eval(x == y) and the test would still
test the same thing. All tests here that expect a concrete integer in the last
check should probably be just converted to clang_analyzer_eval tests.
https://reviews.llvm.org/D41938
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
