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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:8879
+  if (IsComparisonConstant)
     return AnalyzeImpConvsInComparison(S, E);
   
----------------
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.


================
Comment at: lib/Sema/SemaChecking.cpp:8589
+  Expr *LHS = E->getLHS()->IgnoreParenImpCasts();
+  Expr *RHS = E->getRHS()->IgnoreParenImpCasts();
+
----------------
Do you still need these?  I'm always antsy about code that ignores implicit casts, especially before evaluation.


Repository:
  rL LLVM

https://reviews.llvm.org/D37565