manas added a comment. In D106102#3029633 <https://reviews.llvm.org/D106102#3029633>, @martong wrote:
> In D106102#3028584 <https://reviews.llvm.org/D106102#3028584>, @manas wrote: > >> The pre-merge checks fail due to the patch being unable to get applied. The >> troubleshooting >> <https://buildkite.com/llvm-project/diff-checks/builds/65987#annotation-patch_diff> >> suggest to update the patch via `arc diff ```git merge-base HEAD origin``` >> --update D106102` and it would include all local changes into that patch. >> Shouldn't I avoid including all local changes, as some local changes are >> still WIP? > > I guess `HEAD` should refer to the commit that represents this patch, in this > case that is `ce82443c69be`. And I believe you local changes that are not > committed should not affect `arc diff` at all. But, as a precaution, you > could first stash your local changes before calling `arc`. Hope this helps. Thanks. That was helpful. ================ Comment at: clang/test/Analysis/constant-folding.c:470 + +void testEqualityRules(unsigned int a, unsigned int b, int c, int d) { + // Checks when ranges are not overlapping ---------------- steakhal wrote: > steakhal wrote: > > > I would prefer `u1`, `u2`, `s1`, `s2` respectively. > This way the name would signify the signess of the variable. Makes sense. I have amended it. ================ Comment at: clang/test/Analysis/constant-folding.c:473 + if (a <= 10 && b >= 20) { + clang_analyzer_eval((a != b) != 0); // expected-warning{{TRUE}} + } ---------------- steakhal wrote: > Isn't `a != b` enough? Why do you need the `(..) != 0` part? Sometime ago, Valeriy and I were discussing about how solver can't reason about sub-expressions containing logical operators with ranges. Currently, only `==/!=` operators can reason about them, as they are trivial. So, this case is for `!=` operator used with sub-expressions. ================ Comment at: clang/test/Analysis/constant-folding.c:476-478 + if (c <= INT_MIN + 10 && d >= INT_MAX - 10) { + clang_analyzer_eval((c != d) == 0); // expected-warning{{FALSE}} + } ---------------- steakhal wrote: > How is this different compared to the previous case? The difference I can see > is that now we use different constants and the `==` operator in the outer > expression. None of which should really change the behavior AFAICT. I wanted to put a signed case. Although I believe, I should have replaced `==` with `!=`. ================ Comment at: clang/test/Analysis/constant-folding.c:481 + // Checks when ranges are completely overlapping and have more than one point + if (a >= 20 && a <= 50 && b >= 20 && b <= 50) { + clang_analyzer_eval((a != b) != 0); // expected-warning{{UNKNOWN}} ---------------- steakhal wrote: > You could have a comment that `a: [20,50] b:[20,50]`. > It would be easier to comprehend than the chain of conjunctions. > Similarly, how at L464 does. Sure. I have amended. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106102/new/ https://reviews.llvm.org/D106102 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
