Page MenuHomePhabricator

[clang-tidy] Add obvious module for obvious bugs
AbandonedPublic

Authored by Prazek on Dec 15 2016, 9:12 AM.

Details

Summary

Module for checks for obvious bugs that probably won't be found
in working code, but can be found while looking for an
obvious bug in broken code.

Event Timeline

Prazek updated this revision to Diff 81596.Dec 15 2016, 9:12 AM
Prazek retitled this revision from to [clang-tidy] Add obvious module for obvious bugs.
Prazek updated this object.
Prazek added reviewers: alexfh, malcolm.parsons.
Prazek added a subscriber: cfe-commits.

I'm not sure that this is good name for module.

Singe reason for this is check for STL algorithms, may be stl module is more correct destination?

I'm not sure that this is good name for module.

Singe reason for this is check for STL algorithms, may be stl module is more correct destination?

The reason for this module is that I want to make a bunch of clang-tidy checks that are designed to help programmer find a bug in the recently written code. These check probably won't find any bugs in projects, because with example of the first check, either:
a) the code is dead
b) code is correct because we use 2 references to the same vector

in any other case the app would crash, so it only make sense to run this check while programming and looking for the bugs.

Other example of the check I want to write:

for (int i = 0; i < n; i++) {
   for (int j = 0; i < n; j++) {

These are kinds of bugs that with 99% chances will be found during development, so the goal is to reduce programmer time ti find these kind of bugs.

There are many other checks or compiler warnings which could be classified as obvious bugs.

There are many other checks or compiler warnings which could be classified as obvious bugs.

Sure, but unfortunatelly they won't be able to become warning because of false positive rate.
In the example I mentioned this code could be totally valid.

I am really not keen on the name "obvious" for this module. What is obvious to one person is not always obvious to another. Also, if the checks are finding *obvious bugs*, then that suggests they should be implemented in the clang frontend rather than a tool that is run less frequently.

Since the checks are meant to be finding bugs rather than enforcing a coding standard, performance, readability, etc, a few possible alternative names:

bugs-
correctness-

I am really not keen on the name "obvious" for this module. What is obvious to one person is not always obvious to another. Also, if the checks are finding *obvious bugs*, then that suggests they should be implemented in the clang frontend rather than a tool that is run less frequently.

Since the checks are meant to be finding bugs rather than enforcing a coding standard, performance, readability, etc, a few possible alternative names:

bugs-
correctness-

My intention is to have kind of bugs that you facepalm when you find it. Mostly simple typos.
As I said this can't make it to clang because it would have some false positives.

I am ok with making group like bugs that would have some checks that are curently in misc (misc-use-after-move), but still, this group would be special in the sense that it looks for very simple bugs that you can make during coding, but you won't be able to find it in code that seem to work.

I am really not keen on the name "obvious" for this module. What is obvious to one person is not always obvious to another. Also, if the checks are finding *obvious bugs*, then that suggests they should be implemented in the clang frontend rather than a tool that is run less frequently.

Since the checks are meant to be finding bugs rather than enforcing a coding standard, performance, readability, etc, a few possible alternative names:

bugs-
correctness-

My intention is to have kind of bugs that you facepalm when you find it. Mostly simple typos.
As I said this can't make it to clang because it would have some false positives.

Many warnings in clang have some false positives. Do you envision these having a fairly high false positive rate, or expensive to check for?

I am ok with making group like bugs that would have some checks that are curently in misc (misc-use-after-move), but still, this group would be special in the sense that it looks for very simple bugs that you can make during coding, but you won't be able to find it in code that seem to work.

I'm having a hard time imagining how many checks would be in this module from that description. Are you expecting to move items from misc to populate this module (if so, which ones, aside from misc-use-after-move?), or are you intending to add some checks we don't currently have (if so, can you list a few of them)? From your description, it sounds like these are not obvious bugs (since the code seems to work and the bugs are hard to spot), but it may be that "bugs" or "correctness" could also be a poor name. With some examples of checks that could go in the module, we may be able to pick a more appropriate name.

The provided example (typoing "i" for "j") sounds like the sort of thing that PVS-Studio catches; maybe see what wording they use for that kind of mistake? Without investigating, I would suggest "cut-and-paste-error" or "likely-typo".

However, the attached patch *doesn't implement* the provided example diagnostic, so right now it's really hard to say what the intent of this patch *is*. It's hard to argue about whether the name "obvious-" is correct for this group of diagnostics when the group is, er, empty. I suggest that the way forward is to implement some of the intended diagnostics under an existing group and then later refactor them into a new group as needed; or else to implement so many of the intended diagnostics in *this* patch that the necessity for the new group is "obvious" :) to everyone involved.

Coincidentally, I was just reading https://en.wikipedia.org/wiki/WP:LEADFOLLOWSBODY last night. I think it's applicable here. :)

The example of this kind of check is here:
https://reviews.llvm.org/D27806

I am not sure if it make sense to put it as clang warning.

After a little bit of thinking I guess name "typos" would be better, because I want to look for checks that are mostly typos (which are obvious btw) but it would make it more concrete.

The example of this kind of check is here:
https://reviews.llvm.org/D27806

I am not sure if it make sense to put it as clang warning.

After a little bit of thinking I guess name "typos" would be better, because I want to look for checks that are mostly typos (which are obvious btw) but it would make it more concrete.

When I hear "typo", I think "misspelling" and wonder how it differs from the typo correction that clang frontend has. I've usually heard of what your linked review covers as a "thinko" (https://en.wiktionary.org/wiki/thinko) because you made a logical mistake rather than a misspelling, but that's a pretty awful name for the module. ;-)

Perhaps "mistake-", "oops-", "derp-" or something along those lines (well, maybe not "derp")?

PVS-Studio implements tons of checks of this variety. See e.g.
http://www.viva64.com/en/b/0299/#ID0E4KBM
They don't have a catchy name for the category either, but perhaps "suspicious-" or "copypaste-" would do.

I agree with Aaron that "thinko-" would be a little inappropriate for a user-facing name, and besides, I personally would not call this kind of error a "thinko". A thinko stems from a "brain fart", i.e. when you momentarily believe something that in hindsight is obviously false; e.g. writing code for a case that can't ever be hit in practice, or choosing the wrong algorithm because you misunderstood the problem to be solved. The kind of error I'm thinking of in this case stems from not thinking at all, e.g. incomplete copy-pasting, or writing (ch != 'A' || ch != 'a'), or whatever.

Prazek abandoned this revision.Jun 24 2017, 2:59 AM