[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj added a comment.

Okay, but as you can see the majority of my test cases are intentionally false negatives `- -Wsign-conversion` triggers so often than many people don't use it. And, `unsigned x = 2;` does not trigger a sign conversion warning despite there being a conversion form 2 to 2u. This check is targeting a very specific but frequent case of functions that do not guard against containers that might be empty.

Regarding the false positives - I think you are focusing on the semantics of the fix-it which is literally a no-op. The code before and after may be just as wrong, but the salience of the implied conversion is a bit higher. Maybe that's not a good idea for a change that may be applied automatically, even if safe.

In short I'm not sure if you are objecting the principle here, or the implementation. Is this something that can be refined further? I understand the objection in the first case, but what about the second case makes you think this is not a good diagnostic and a false positive? It's the specific case I was targeting: the unprotected subtraction of a non-zero value from a potentially smaller value.

This check was used across our very large code base and was evaluated by examining the code by looking at the context where these warning fired. The vast majority were judged to be true positives. Unfortunately it's not possible to share those results. I will try to run this check on some fraction of the llvm code base and follow up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71607/new/

https://reviews.llvm.org/D71607