[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:8879
+  if (IsComparisonConstant)
     return AnalyzeImpConvsInComparison(S, E);
   
----------------
rjmccall wrote:
> lebedev.ri wrote:
> > rjmccall wrote:
> > > Part of the purpose of checking for signed comparisons up here is to avoid unnecessarily computing ranges for the operands when we aren't going to use them.  That's still something we want to avoid.  I think you just need to call CheckTrivialUnsignedComparison in this fallback case, at least when the comparison is not constant.
> > > 
> > > You should also rename CheckTrivialUnsignedComparison to something like CheckTautologicalComparison.
> > Ok, i must admit that i understand the idea to have minimal overhead. But i don't really follow the code in here :)
> > This seems to work, and `check-clang-sema`+`check-clang-semacxx`+stage2 still pass.
> > Is this obviously-wrong / some testcase is missing? 
> There's a lot of code here doing very similar checks, and we just want to ensure that we aren't doing too much redundant work.  I think the way you've structured it now is fine.
OK :)


================
Comment at: lib/Sema/SemaChecking.cpp:8589
+  Expr *LHS = E->getLHS()->IgnoreParenImpCasts();
+  Expr *RHS = E->getRHS()->IgnoreParenImpCasts();
+
----------------
rjmccall wrote:
> Do you still need these?  I'm always antsy about code that ignores implicit casts, especially before evaluation.
Yes, absolutely. I did check without them, and they are needed. Else:
```
$ ninja check-clang-sema check-clang-semacxx
[27/29] Running lit suite /build/clang/test/Sema
lit.py: /build/clang/test/lit.cfg:200: note: using clang: '/build/llvm-build-Clang-release/./bin/clang'
FAIL: Clang :: Sema/compare.c (195 of 548)
******************** TEST 'Clang :: Sema/compare.c' FAILED ********************
Script:
--
/build/llvm-build-Clang-release/./bin/clang -cc1 -internal-isystem /build/llvm-build-Clang-release/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-darwin -fsyntax-only -pedantic -verify -Wsign-compare /build/clang/test/Sema/compare.c -Wno-unreachable-code
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics seen but not expected: 
  File /build/clang/test/Sema/compare.c Line 80: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/compare.c Line 88: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/compare.c Line 89: comparison of unsigned expression < 0 is always false
3 errors generated.

--

********************
FAIL: Clang :: Sema/outof-range-constant-compare.c (357 of 548)
******************** TEST 'Clang :: Sema/outof-range-constant-compare.c' FAILED ********************
Script:
--
/build/llvm-build-Clang-release/./bin/clang -cc1 -internal-isystem /build/llvm-build-Clang-release/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-darwin -fsyntax-only -Wtautological-constant-out-of-range-compare -verify /build/clang/test/Sema/outof-range-constant-compare.c
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics expected but not seen: 
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 163: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 169: comparison of unsigned expression >= 0 is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 178: comparison of 0 <= unsigned expression is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 180: comparison of 0 > unsigned expression is always false
error: 'warning' diagnostics seen but not expected: 
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 40: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 46: comparison of unsigned expression >= 0 is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 55: comparison of 0 <= unsigned expression is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 57: comparison of 0 > unsigned expression is always false
8 errors generated.

--

********************
Testing Time: 2.05s
********************
Failing Tests (2):
    Clang :: Sema/compare.c
    Clang :: Sema/outof-range-constant-compare.c

  Expected Passes    : 546
  Unexpected Failures: 2
FAILED: tools/clang/test/CMakeFiles/check-clang-sema 
cd /build/llvm-build-Clang-release/tools/clang/test && /usr/bin/python2.7 /build/llvm/utils/lit/lit.py -sv --param clang_site_config=/build/llvm-build-Clang-release/tools/clang/test/lit.site.cfg /build/clang/test/Sema
ninja: build stopped: subcommand failed.
```


Repository:
  rL LLVM

https://reviews.llvm.org/D37565