This is an archive of the discontinued LLVM Phabricator instance.

Add bugprone-bool-to-integer-conversion
AbandonedPublic

Authored by Prazek on Apr 6 2016, 3:28 AM.

Details

Reviewers
alexfh
mnbvmar
Summary

Tested on llvm codebase.
It have found many places like:

  • returning true/false in function returning int,
  • assigning true/false to integer inside some classes.

Diff Detail

Event Timeline

Prazek updated this revision to Diff 52776.Apr 6 2016, 3:28 AM
Prazek retitled this revision from to Add modernize-bool-to-integer-conversion.
Prazek updated this object.
Prazek added reviewers: alexfh, staronj.
Prazek added a subscriber: cfe-commits.
Prazek added a comment.Apr 6 2016, 5:17 AM

Maybe we should merge it with http://reviews.llvm.org/D18745 and name it 'modernize-wrong-literal-cast'. The other question is, will it be better to move it to readability?

krystyna added inline comments.Apr 6 2016, 11:44 AM
docs/clang-tidy/checks/modernize-bool-to-integer-conversion.rst
9

int a = false;

mnbvmar edited edge metadata.Apr 6 2016, 12:24 PM

This check throws a warning also on the conversion to floats (probably very rare ones):

double number = true;

Even though this behavior is correct, the code warns about the implicit conversion to integers.

docs/ReleaseNotes.rst
119

with int literals

Prazek marked an inline comment as done.Apr 6 2016, 2:11 PM

This check throws a warning also on the conversion to floats (probably very rare ones):

double number = true;

Even though this behavior is correct, the code warns about the implicit conversion to integers.

That case is extremaly rare. I will see what I can do

Prazek updated this revision to Diff 52851.Apr 6 2016, 2:44 PM
Prazek edited edge metadata.
Prazek marked an inline comment as done.
Prazek updated this revision to Diff 52855.Apr 6 2016, 2:56 PM

Added new test cases

alexfh edited edge metadata.

Actually, did you think about adding this as a clang diagnostic?

Richard, what do you think about complaining in Clang about int i = true; kind of code?

clang-tidy/modernize/BoolToIntegerConversionCheck.cpp
50

s/. Use/; use/

Same below.

docs/ReleaseNotes.rst
119

The phrase is technically incorrect. The check does not replace implicit cast from bool literals. It replaces bool literals (which are being implicitly cast to integers) with integer literals.

docs/clang-tidy/checks/modernize-bool-to-integer-conversion.rst
10

There should be an empty line after .. code-block ....

Prazek added a comment.Apr 7 2016, 9:06 AM

Actually, did you think about adding this as a clang diagnostic?

Richard, what do you think about complaining in Clang about int i = true; kind of code?

Glad to hear that :)

alexfh added inline comments.Apr 7 2016, 9:15 AM
clang-tidy/modernize/BoolToIntegerConversionCheck.cpp
48

I'd prefer an alternative with less code duplication:

auto Diag = diag("implicitly converting bool literal to %0%select{|inside a macro}1; use ...") << Type;
if (<should add fixes>)
  Diag << 1 << FixItHint::...;
else
  Diag << 0;
alexfh requested changes to this revision.Apr 7 2016, 11:25 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 7 2016, 11:25 AM
Prazek updated this revision to Diff 53113.Apr 9 2016, 2:45 AM
Prazek edited edge metadata.
Prazek marked 2 inline comments as done.
Prazek updated this revision to Diff 53114.Apr 9 2016, 3:50 AM
Prazek edited edge metadata.
Prazek marked 2 inline comments as done.

Used isMacroID to determinate if it's macro

BTW, why is the check in the 'modernize' module? It doesn't seem to make anything more modern. I would guess, the pattern it detects is most likely to result from a programming error. Also, the fix, though it retains the behavior, has a high chance to be incorrect. Can you share the results of running this check on LLVM? At least, how many problems it found and how many times the suggested fix was correct.

I'd suggest to move the check to misc or maybe it's time to create a separate directory for checks targeting various bug-prone patterns.

clang-tidy/modernize/BoolToIntegerConversionCheck.cpp
44

nit: Since both positive and negative branches are present, I'd prefer to make the condition slightly simple by removing the negation.

alexfh requested changes to this revision.Apr 12 2016, 1:37 PM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 12 2016, 1:37 PM

BTW, why is the check in the 'modernize' module? It doesn't seem to make anything more modern. I would guess, the pattern it detects is most likely to result from a programming error. Also, the fix, though it retains the behavior, has a high chance to be incorrect. Can you share the results of running this check on LLVM? At least, how many problems it found and how many times the suggested fix was correct.

I'd suggest to move the check to misc or maybe it's time to create a separate directory for checks targeting various bug-prone patterns.

There were many places. Most of them where when assigning true/false to some member which was int.
The other place was inside some functions that were returning only true or false, but the return type of this function was int, so I guess what programmer meant there was to return bool.

BTW, why is the check in the 'modernize' module? It doesn't seem to make anything more modern. I would guess, the pattern it detects is most likely to result from a programming error. Also, the fix, though it retains the behavior, has a high chance to be incorrect. Can you share the results of running this check on LLVM? At least, how many problems it found and how many times the suggested fix was correct.

I'd suggest to move the check to misc or maybe it's time to create a separate directory for checks targeting various bug-prone patterns.

There were many places.

Would be nice, if you could tell the number and provide a list of locations. Have any of these been fixed since then?

BTW, why is the check in the 'modernize' module? It doesn't seem to make anything more modern. I would guess, the pattern it detects is most likely to result from a programming error. Also, the fix, though it retains the behavior, has a high chance to be incorrect. Can you share the results of running this check on LLVM? At least, how many problems it found and how many times the suggested fix was correct.

I'd suggest to move the check to misc or maybe it's time to create a separate directory for checks targeting various bug-prone patterns.

There were many places.

Would be nice, if you could tell the number and provide a list of locations. Have any of these been fixed since then?

No, I didn't know that I can do that - I always heard that I can't format code that I didn't change, so I though the same thing is here. I will try do post review of changes in clang/llvm soon.

http://reviews.llvm.org/D19105
Here is a diff containing fixes for clang. What I see is that it would be nice to detect bitfields of 1 bit and treet it as bool, so it won't warn it such cases.

Prazek updated this revision to Diff 53909.Apr 15 2016, 10:11 AM
Prazek updated this object.
Prazek edited edge metadata.
Prazek marked an inline comment as done.

I will think name for new module that would have all the checks like this.
I added ingnoring of bitfields of size 1

BTW, why is the check in the 'modernize' module? It doesn't seem to make anything more modern. I would guess, the pattern it detects is most likely to result from a programming error. Also, the fix, though it retains the behavior, has a high chance to be incorrect. Can you share the results of running this check on LLVM? At least, how many problems it found and how many times the suggested fix was correct.

I'd suggest to move the check to misc or maybe it's time to create a separate directory for checks targeting various bug-prone patterns.

Do you have any thought about the name for such a module? I belive that misc is overloaded.

So for this we are looking for something that is probably not a bug, but it makes code a little bit inaccurate
Maybe something like:

  • accuracy,
  • correctness,
  • certainity,
  • safety,
  • maybebugmaybenothardtosay

I would like to see a new version of http://reviews.llvm.org/D19105 with all the "1-bit-bitfield" diffs removed.
Right now, it's hard to see that there's *anything* in D19105 that's not a miscorrection of a 1-bit bitfield.

Do you have an example of a large codebase where this check runs with few false positives and a non-zero number of true positives?

I would like to see a new version of http://reviews.llvm.org/D19105 with all the "1-bit-bitfield" diffs removed.
Right now, it's hard to see that there's *anything* in D19105 that's not a miscorrection of a 1-bit bitfield.

Do you have an example of a large codebase where this check runs with few false positives and a non-zero number of true positives?

I will post fixex tomorrow. I don't think there will be any false or true positives in warnings, but the main problems with fixes are that many times developer wants to use bools, but it decalres field/return type as int.

BTW, why is the check in the 'modernize' module? It doesn't seem to make anything more modern. I would guess, the pattern it detects is most likely to result from a programming error. Also, the fix, though it retains the behavior, has a high chance to be incorrect. Can you share the results of running this check on LLVM? At least, how many problems it found and how many times the suggested fix was correct.

I'd suggest to move the check to misc or maybe it's time to create a separate directory for checks targeting various bug-prone patterns.

Do you have any thought about the name for such a module? I belive that misc is overloaded.

So for this we are looking for something that is probably not a bug, but it makes code a little bit inaccurate
Maybe something like:

  • accuracy,
  • correctness,
  • certainity,
  • safety,
  • maybebugmaybenothardtosay

after a long though I think that "accuracy" is the best name here - we want to look for a code that is valid, but not accurate

alexfh requested changes to this revision.Apr 26 2016, 7:04 AM
alexfh edited edge metadata.

BTW, why is the check in the 'modernize' module? It doesn't seem to make anything more modern. I would guess, the pattern it detects is most likely to result from a programming error. Also, the fix, though it retains the behavior, has a high chance to be incorrect. Can you share the results of running this check on LLVM? At least, how many problems it found and how many times the suggested fix was correct.

I'd suggest to move the check to misc or maybe it's time to create a separate directory for checks targeting various bug-prone patterns.

Do you have any thought about the name for such a module? I belive that misc is overloaded.

So for this we are looking for something that is probably not a bug, but it makes code a little bit inaccurate
Maybe something like:

  • accuracy,
  • correctness,
  • certainity,
  • safety,
  • maybebugmaybenothardtosay

after a long though I think that "accuracy" is the best name here - we want to look for a code that is valid, but not accurate

There are many possible reasons this pattern can appear in the code. Sometimes it's a bug, sometimes, it's an attempt to make the code look better than it is ;) It seems to me though, that having a type mismatch of this kind is a bug-prone pattern, so I would start a more generic "bugprone" category of checks (and move some misc- checks there later on).

Also, please re-upload D19105 after disabling matches on 1-bit bitfields in the check.

This revision now requires changes to proceed.Apr 26 2016, 7:04 AM
Prazek updated this revision to Diff 56138.May 4 2016, 6:31 AM
Prazek retitled this revision from Add modernize-bool-to-integer-conversion to Add bugprone-bool-to-integer-conversion.
Prazek edited edge metadata.

It seems that it works right now.
The other funny thing that the check found is cases like

bool b;
if (b == true)

I will update the llvm/clang changes today.
Do you want to see only the changes that clang-tidy made, or do you want to see it after some changing that I will do?

Prazek added a comment.May 5 2016, 2:45 AM

Updated the diff with changes
http://reviews.llvm.org/D19105

alexfh requested changes to this revision.May 17 2016, 12:14 PM
alexfh edited edge metadata.
alexfh added inline comments.
docs/clang-tidy/checks/bugprone-bool-to-integer-conversion.rst
43 ↗(On Diff #56138)

This is a false positive and should be fixed regardless of whether we disable fixits or not. Just documenting it doesn't help much.

This revision now requires changes to proceed.May 17 2016, 12:14 PM
Prazek updated this revision to Diff 58148.May 23 2016, 1:57 PM
Prazek edited edge metadata.

+Fixed bug with operators
+ added fixup for function return type

I will post changes on clang tomorrow

Prazek updated this revision to Diff 58309.May 24 2016, 1:48 PM
Prazek edited edge metadata.

Some small bugfixes afeter running it on llvm

Prazek updated this revision to Diff 58961.May 30 2016, 8:23 AM

aborting with virtual functions

alexfh requested changes to this revision.Jun 3 2016, 6:06 PM
alexfh edited edge metadata.
alexfh added inline comments.
docs/clang-tidy/checks/bugprone-bool-to-integer-conversion.rst
37 ↗(On Diff #58961)

It may well not be a bug. It's definitely not a bug for extern "C" functions and functions in C code. We definitely don't need to change the function return type, since it's a rather involved change (and clang-tidy checks currently are simply not able to make such change correctly in case of a function with external linkage). So please remove this automated fix.

41 ↗(On Diff #58961)

Please check that your documentation compiles.

test/clang-tidy/bugprone-bool-to-integer-conversion.cpp
191 ↗(On Diff #58309)

That's a dangerous fix. It should either be removed or guarded by an option.

This revision now requires changes to proceed.Jun 3 2016, 6:06 PM
Prazek added inline comments.Jun 4 2016, 1:37 AM
docs/clang-tidy/checks/bugprone-bool-to-integer-conversion.rst
37 ↗(On Diff #58961)

This check runs only on C++ functions. Maybe checking that the function isExternC() would be enough?

test/clang-tidy/bugprone-bool-to-integer-conversion.cpp
192 ↗(On Diff #58961)

Why it is dangerous? What I see after runnign on llvm code base, that it is one of the most frequent case.
The only bug is the extern "C" thing.

Sorry for the delay. Your patch was lost in my inbox. Feel free to ping me earlier.

docs/clang-tidy/checks/bugprone-bool-to-integer-conversion.rst
37 ↗(On Diff #58961)

As I've said, extern "C" is not the only way to make a function a part of an API exposed to external users.

test/clang-tidy/bugprone-bool-to-integer-conversion.cpp
192 ↗(On Diff #58961)

Well, extern "C" is not the only way to expose functions to some external code. This fix potentially changes ABI, which is generally not a safe thing to do. I think, we should make the fix optional.

Prazek abandoned this revision.Feb 13 2017, 1:59 AM