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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
