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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71607#1828055 <https://reviews.llvm.org/D71607#1828055>, @sorenj wrote:

> 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.


Then we should be addressing that issue rather than duplicating the functionality, no?

> And, `unsigned x = 2;` does not trigger a sign conversion warning despite there being a conversion form 2 to 2u.

That should *not* trigger a sign conversion warning because there is no sign conversion. We know the exact value of the RHS and can see that it does not change signs.

> This check is targeting a very specific but frequent case of functions that do not guard against containers that might be empty.

We should consider a better name for the check then and limit its utility to those cases. Truthfully, I think that check would be quite useful, but it would almost certainly be a clang static analyzer check to handle control and data flow. e.g., such a check should be able to handle these situations:

  size_t count1 = some_container.size() - 1; // Bad if empty
  size_t num_elements = some_container.size();
  size_t count2 = num_elements - 1; // Bad if empty
  if (num_element)
    size_t count3 = num_elements - 1; // Just fine
  size_t count4 = std::size(some_container) - 1; // Bad if empty
  size_t count5 = std::distance(some_container.begin(), some_container.end()) - 1; // Bad if empty? (Note, there's no type-based sign conversion here)
  
  num_elements + 1;
  size_t count6 = num_elements - 1; // Just fine



> 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.

A bit of both. I don't think clang-tidy should get a -Wsign-conversion check -- the frontend handles that, and if there are deficiencies with that handling, we should address those. However, a check that's focused solely on container and container-like situations where an empty container would cause problems seems like a very interesting check that has value. I'm not certain that implementing it in clang-tidy would catch the cases with a reasonable false-positive rate, but it seems like it wouldn't be bad as a static analyzer check instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607