[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision now requires review to proceed.
Herald added subscribers: carlosgalvezp, StephenFan.
Herald added a reviewer: njames93.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


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

https://reviews.llvm.org/D59859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added inline comments.
This revision now requires changes to proceed.



Comment at: 
clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp:270
+  AllowPointerConditions(Options.get("AllowPointerConditions", false)),
+  
UseUppercaseLiteralSuffix(Context->isCheckEnabled("readability-uppercase-literal-suffix"))
 {}
 

alexfh wrote:
> Relying on enabled checks for this sort of a behavior is undesired. This sort 
> of an implicit dependency will lead to problems, if, for example, a user 
> wants to apply fixes check-by-check. A separate option (local for this check 
> or better a global one - like `StrictMode`) would be better.
Looks like this wasn't resolved - it should still be a config option, not 
hardcoded.


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

https://reviews.llvm.org/D59859



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Patrick Nappa via Phabricator via cfe-commits]

pnappa updated this revision to Diff 196795.
pnappa added a comment.

Apologies for taking a while, I'm afraid I hadn't had a moment of spare time 
over the past few weeks.

I believe I've applied the requested changes, bar one that I'm unsure about:

@alexfh to be sure, would adding an Option to 
"readability-implicit-bool-conversion" called say, "EnforceUppercase", suffice? 
With that option enabled the synthesis of the explicit comparison would be 
uppercase.

I'll make the appropriate changes to documentation when I get everything 
finalised.


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

https://reviews.llvm.org/D59859

Files:
  clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
  clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h
  clang-tools-extra/test/clang-tidy/readability-implicit-bool-conversion.cpp
  
clang-tools-extra/test/clang-tidy/readability-uppercase-literal-suffix-implicit-bool-conversion.cpp

Index: clang-tools-extra/test/clang-tidy/readability-uppercase-literal-suffix-implicit-bool-conversion.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-uppercase-literal-suffix-implicit-bool-conversion.cpp
@@ -0,0 +1,124 @@
+// RUN: %check_clang_tidy %s readability-uppercase-literal-suffix,readability-implicit-bool-conversion %t
+
+template
+void functionTaking(T);
+
+
+// Implicit conversion to bool with enforced uppercase suffix.
+
+void implicitConversionToBoolSimpleCases() {
+  unsigned int unsignedInteger = 10u;
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: integer literal has suffix 'u', which is not uppercase
+  // CHECK-MESSAGES-NEXT: unsigned int unsignedInteger = 10u;
+  // CHECK-MESSAGES-NEXT: ^ ~
+  // CHECK-MESSAGES-NEXT: {{^ *}}U{{$}}
+  // CHECK-FIXES: unsigned int unsignedInteger = 10U;
+
+  functionTaking(unsignedInteger);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'unsigned int' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking(unsignedInteger != 0U);
+
+  unsigned long unsignedLong = 10ul;
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: integer literal has suffix 'ul', which is not uppercase [readability-uppercase-literal-suffix]
+  // CHECK-MESSAGES-NEXT: unsigned long unsignedLong = 10ul;
+  // CHECK-MESSAGES-NEXT: ^ ~~
+  // CHECK-MESSAGES-NEXT: {{^ *}}UL{{$}}
+  // CHECK-FIXES: unsigned long unsignedLong = 10UL;
+
+  functionTaking(unsignedLong);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'unsigned long' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking(unsignedLong != 0U);
+
+  float floating = 10.0f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
+  // CHECK-MESSAGES-NEXT: float floating = 10.0f;
+  // CHECK-MESSAGES-NEXT: ^   ~
+  // CHECK-MESSAGES-NEXT: {{^ *}}F{{$}}
+  // CHECK-FIXES: float floating = 10.0F;
+
+  functionTaking(floating);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'float' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking(floating != 0.0F);
+
+  double doubleFloating = 10.0;
+  functionTaking(doubleFloating);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'double' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking(doubleFloating != 0.0);
+}
+
+void implicitConversionToBoolInComplexExpressions() {
+  bool boolean = true;
+
+  unsigned int integer = 10U;
+  unsigned int anotherInteger = 20U;
+  bool boolComingFromUnsignedInteger = integer + anotherInteger;
+  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: implicit conversion 'unsigned int' -> bool
+  // CHECK-FIXES: bool boolComingFromUnsignedInteger = (integer + anotherInteger) != 0U;
+
+  float floating = 0.2F;
+  // combination of two errors on the same line
+  bool boolComingFromFloating = floating - 0.3f || boolean;
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: implicit conversion 'float' -> bool
+  // CHECK-MESSAGES: :[[@LINE-2]]:44: warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
+  // CHECK-FIXES: bool boolComingFromFloating = ((floating - 0.3F) != 0.0F) || boolean;
+}
+
+void implicitConversionInNegationExpressions() {
+  // ensure that in negation the replaced suffix is also capitalized
+  float floating = 10.0F;
+  bool boolComingFromNegatedFloat = ! floating;
+  // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: implicit conversion 'float' -> bool
+  // CHECK-FIXES: bool boolComingFromNegatedFloat = floating == 0.0F;
+}
+
+void implicitConversionToBoolInControlStatements() {
+  unsigned long longInteger = 2U;
+  for (;longInteger;) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: implicit conversion 'unsigned long' -> bool
+  // CHECK-FIXES: for (;longInteger != 

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp:270
+  AllowPointerConditions(Options.get("AllowPointerConditions", false)),
+  
UseUppercaseLiteralSuffix(Context->isCheckEnabled("readability-uppercase-literal-suffix"))
 {}
 

Relying on enabled checks for this sort of a behavior is undesired. This sort 
of an implicit dependency will lead to problems, if, for example, a user wants 
to apply fixes check-by-check. A separate option (local for this check or 
better a global one - like `StrictMode`) would be better.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59859



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp:48
   case CK_IntegralToBoolean:
-return Type->isUnsignedIntegerType() ? "0u" : "0";
+if (UppercaseSuffix) {
+  return Type->isUnsignedIntegerType() ? "0U" : "0";

MyDeveloperDay wrote:
> LLVM normally doesn't put braces on small lines
Maybe use a conditional operator instead?

  return Type->isUnsignedIntegerType() ? (UppercaseSuffix ? "0U" : "0u") : "0";


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59859



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D59859#1444333 , @aaron.ballman 
wrote:

> In D59859#1444176 , @lebedev.ri 
> wrote:
>
> > I'm not sure why we want this? What is wrong with simply applying 
> > clang-tidy twice?
>
>
> It doubles the execution time of checking a large project (which may be 
> unacceptably slow), and there's no guarantee that twice will be enough in the 
> general case (one set of fixes may trigger a different check's diagnostics, 
> which may get fixed and trigger another check's diagnostics, etc).


Yeah, the situation when one automated fix generates another warning is not a 
nice user experience. It's not only about clang-tidy run time, it's also about 
annoying users and making their workflow less efficient.  Thus it's better to 
generate clang-tidy-clean code in fixes, where possible and not particularly 
difficult to implement.

As for the readability-uppercase-literal-suffix check (, I wonder whether 
clang-format could do this? Non-whitespace changes were historically not 
desired in clang-format, but there are a number of features there that generate 
non-whitespace changes (e.g. comment formatting, string literal splitting, 
#include sorting, macro formatting). This one seems to be local and quite safe. 
Maybe propose this feature to clang-format?

If the clang-format feature is viable, clang-tidy can already apply 
clang-format formatting. If not, I'm also not opposed to the new option for 
readability-implicit-bool-conversion (but maybe it should be a global option 
like `StrictMode`)?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59859



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

We normally add something to the documentation about the checker and/or the 
release notes to say what had changed




Comment at: 
clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp:48
   case CK_IntegralToBoolean:
-return Type->isUnsignedIntegerType() ? "0u" : "0";
+if (UppercaseSuffix) {
+  return Type->isUnsignedIntegerType() ? "0U" : "0";

LLVM normally doesn't put braces on small lines



Comment at: 
clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp:221
+return BoolLiteral->getValue() ? "1.0F" : "0.0F";
+  } else {
+return BoolLiteral->getValue() ? "1.0f" : "0.0f";

your don't really need the else here because you return above it


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59859



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: alexfh.
aaron.ballman added a subscriber: alexfh.
aaron.ballman added a comment.

> Intended as my first commit to the llvm-project.

Welcome! Thank you for working on this!

In D59859#1444176 , @lebedev.ri wrote:

> Please always upload all patches with full context (`-U9`)
>  I'm not sure why we want this? What is wrong with simply applying clang-tidy 
> twice?


It doubles the execution time of checking a large project (which may be 
unacceptably slow), and there's no guarantee that twice will be enough in the 
general case (one set of fixes may trigger a different check's diagnostics, 
which may get fixed and trigger another check's diagnostics, etc).

However, it seems like we can run into this sort of interaction in many 
different checks and in many different ways, and I am not certain that we 
should try to tackle the maintenance burden of dealing with those interactions 
ad hoc like this. That said, I'm not certain of a more principled solution 
either. Adding @alexfh to see if he has put any thought into this area and has 
ideas.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59859



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a reviewer: aaron.ballman.
lebedev.ri added a comment.

Please always upload all patches with full context (`-U9`)
I'm not sure why we want this? What is wrong with simply applying clang-tidy 
twice?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59859



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Patrick Nappa via Phabricator via cfe-commits]

pnappa created this revision.
pnappa added a reviewer: lebedev.ri.
pnappa added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, jdoerfert, xazax.hun.
Herald added a project: clang.

Fix for https://bugs.llvm.org/show_bug.cgi?id=41199#c1

Previously, if a user implicitly cast to a bool (say, in a conditional 
statement), the fix would be to add an explicit comparison. So, for a floating 
point implicit case to bool, from `if (f)`, the synthesised code would be `if 
(f != 0.0f)`.

Even if the flag "readability-uppercase-literal-suffix" was enabled, the 
synthesised suffix would be lowercase. This commit changes that, such that if 
that flag is enabled when "readability-implicit-bool-conversion" is enabled, 
the synthesised suffix is uppercase.

A non-covered case is when "modernize-use-default-member-init" is enabled, 
lower-case suffixes may still be synthesised (I think, based off the code). Any 
RFC whether this should be made a different issue or whether or not this 
behaviour should be added in?

Intended as my first commit to the llvm-project.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D59859

Files:
  clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
  clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h
  clang-tools-extra/test/clang-tidy/readability-implicit-bool-conversion.cpp
  
clang-tools-extra/test/clang-tidy/readability-uppercase-literal-suffix-implicit-bool-conversion.cpp

Index: clang-tools-extra/test/clang-tidy/readability-uppercase-literal-suffix-implicit-bool-conversion.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-uppercase-literal-suffix-implicit-bool-conversion.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s readability-uppercase-literal-suffix,readability-implicit-bool-conversion %t
+
+template
+void functionTaking(T);
+
+
+// Implicit conversion to bool with enforced uppercase suffix.
+
+// TODO: should we add complex number suffix tests? I don't see any here. It may be an annoyance thing due to it's a C++17 test. I should ask in phabricator when I submit.
+// XXX: ensure that tabs are 2 wide
+// XXX: run clang tidy on this using the clangtidy config at the root of the repo
+void implicitConversionToBoolSimpleCases() {
+  unsigned int unsignedInteger = 10u;
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: integer literal has suffix 'u', which is not uppercase
+  // CHECK-MESSAGES-NEXT: unsigned int unsignedInteger = 10u;
+  // CHECK-MESSAGES-NEXT: ^ ~
+  // CHECK-MESSAGES-NEXT: {{^ *}}U{{$}}
+  // CHECK-FIXES: unsigned int unsignedInteger = 10U;
+
+  functionTaking(unsignedInteger);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'unsigned int' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking(unsignedInteger != 0U);
+
+  unsigned long unsignedLong = 10ul;
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: integer literal has suffix 'ul', which is not uppercase [readability-uppercase-literal-suffix]
+  // CHECK-MESSAGES-NEXT: unsigned long unsignedLong = 10ul;
+  // CHECK-MESSAGES-NEXT: ^ ~~
+  // CHECK-MESSAGES-NEXT: {{^ *}}UL{{$}}
+  // CHECK-FIXES: unsigned long unsignedLong = 10UL;
+
+  functionTaking(unsignedLong);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'unsigned long' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking(unsignedLong != 0U);
+
+  float floating = 10.0f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
+  // CHECK-MESSAGES-NEXT: float floating = 10.0f;
+  // CHECK-MESSAGES-NEXT: ^   ~
+  // CHECK-MESSAGES-NEXT: {{^ *}}F{{$}}
+  // CHECK-FIXES: float floating = 10.0F;
+
+  functionTaking(floating);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'float' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking(floating != 0.0F);
+
+  double doubleFloating = 10.0;
+  functionTaking(doubleFloating);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'double' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking(doubleFloating != 0.0);
+}
+
+void implicitConversionToBoolInComplexExpressions() {
+  bool boolean = true;
+
+  unsigned int integer = 10U;
+  unsigned int anotherInteger = 20U;
+  bool boolComingFromUnsignedInteger = integer + anotherInteger;
+  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: implicit conversion 'unsigned int' -> bool
+  // CHECK-FIXES: bool boolComingFromUnsignedInteger = (integer + anotherInteger) != 0U;
+
+  float floating = 0.2F;
+  // combination of two errors on the same line
+  bool boolComingFromFloating = floating - 0.3f || boolean;
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: implicit conversion 'float' -> bool
+  // CHECK-MESSAGES: :[[@LINE-2]]:44: warning: