ASDenysPetrov marked 9 inline comments as done.
ASDenysPetrov added a comment.
@baloghadamsoftware
> However, please still extend the current tests
I've looked for some appropriate tests to extend, but haven't found. What would
you advise to use instead of mine?
@steakhal
Thanks for the comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:217
+
+ const llvm::APSInt sampleValue = getMinValue();
+ const bool isUnsigned = sampleValue.isUnsigned();
----------------
steakhal wrote:
> Should we take it as `const ref` to prevent copying?
getMinValue returns APSInt by value, so it wouldn't make sense.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:572-573
if (const RangeSet *negV = State->get<ConstraintRange>(negSym)) {
- // Unsigned range set cannot be negated, unless it is [0, 0].
- if ((negV->getConcreteValue() &&
- (*negV->getConcreteValue() == 0)) ||
+ if (T->isUnsignedIntegerOrEnumerationType() ||
T->isSignedIntegerOrEnumerationType())
return negV;
----------------
steakhal wrote:
> Couldn't we simplify the disjunction to single `T->isEnumerationType()` or to
> something similar?
I've looked for similar single function, but there is no appropriate one. I
decided not to add a new one to make patch more focused.
================
Comment at: clang/test/Analysis/constraint_manager_negate_difference.c:114-118
+void negated_unsigned_range(unsigned x, unsigned y) {
+ clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}}
expected-warning{{TRUE}}
+ clang_analyzer_eval(y - x != 0); // expected-warning{{FALSE}}
expected-warning{{TRUE}}
+ clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}}
expected-warning{{TRUE}}
+}
----------------
steakhal wrote:
> What does this test case demonstrate? Could you elaborate on that?
> Why do you evaluate the `x - y != 0` here twice?
>Why do you evaluate the x - y != 0 here twice?
This is the root line which assertion occured on.
I'll add some comments.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:23
+template <typename T> struct TestCase {
+ RangeSet original;
+ RangeSet expected;
----------------
steakhal wrote:
> According to the [LLVM coding
> style](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)
> we should use UpperCamelCase for variable names for new code.
>
> Note that the Analyzer Core was written in a different style, we should
> follow that there instead (at least IMO).
Here is a discussion [[ https://llvm.org/docs/Proposals/VariableNames.html |
VariableNames]] I was guide. Seems like we can use camelBack for new code. Am I
right? Also RangeSetTest.cpp already uses mixed naming as you can see.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:27-28
+ TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+ const std::initializer_list<T> &originalList,
+ const std::initializer_list<T> &expectedList)
+ : original(createRangeSetFromList(BVF, F, originalList)),
----------------
steakhal wrote:
> AFAIK since `std::initializer_list` is just two pointers we should take it by
> value.
I'm not sure about //should//, since initializer_list is a container and it'd
be consistent if we handle it like any other compound object despite its
implementation (IMO). But I can change it if you wish.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:51
+ // init block
+ DiagnosticsEngine Diag{new DiagnosticIDs, new DiagnosticOptions};
+ FileSystemOptions FileSystemOpts;
----------------
steakhal wrote:
> Generally, `new expressions` are a code smell. We should use something like
> an `std::make_unique` to prevent memory leaks on exceptions.
> Though, I'm not sure if there is a similar function for
> `llvm::IntrusiveRefCntPtr<T>`s.
I'll make it more safe.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:66
+ template <typename T> void checkNegate() {
+ using type = T;
+
----------------
steakhal wrote:
> To be honest, I'm not sure if `type` is more descriptive than `T`.
It is useful for debug purposes, for instance you can change it to `using type
= int;` to verify something. It also untie from template argument list and make
the code more flexible.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:68
+
+ // use next values of the range {MIN, A, B, MID, C, D, MAX}
+
----------------
steakhal wrote:
> AFAIK full sentences are required for comments.
> https://llvm.org/docs/CodingStandards.html#commenting
I'll correct it.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:113-120
+ // c.original.print(llvm::dbgs());
+ // llvm::dbgs() << " => ";
+ // c.expected.print(llvm::dbgs());
+ // llvm::dbgs() << " => ";
+ // negatedFromOriginal.print(llvm::dbgs());
+ // llvm::dbgs() << " => ";
+ // negatedBackward.print(llvm::dbgs());
----------------
steakhal wrote:
> Should we keep this?
I'm not sure, but I'd better keep it, because it is useful for debugging.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77802/new/
https://reviews.llvm.org/D77802
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
