[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5bbe50148f3b: [clang-tidy] Warn on functional C-style casts 
(authored by carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114427

Files:
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
@@ -143,11 +143,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant cast to the same type
   // CHECK-FIXES: {{^}}  kZero;
 
-  int b2 = int(b);
-  int b3 = static_cast(b);
-  int b4 = b;
+  int b2 = static_cast(b);
+  int b3 = b;
   double aa = a;
-  (void)b2;
+  (void)aa;
   return (void)g();
 }
 
@@ -321,3 +320,50 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: C-style casts are discouraged; use constructor call syntax [
   // CHECK-FIXES: auto s6 = S(cr);
 }
+
+template 
+T functional_cast_template_used_by_class(float i) {
+  return T(i);
+}
+
+template 
+T functional_cast_template_used_by_int(float i) {
+  return T(i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: return static_cast(i);
+}
+
+struct S2 {
+  S2(float);
+};
+using T = S2;
+using U = S2 &;
+
+void functional_casts() {
+  float x = 1.5F;
+  auto y = int(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: auto y = static_cast(x);
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wc++11-narrowing"
+  // This if fine, compiler will warn about implicit conversions with brace initialization
+  auto z = int{x};
+#pragma clang diagnostic pop
+
+  // Functional casts are allowed if S is of class type
+  const char *str = "foo";
+  auto s = S(str);
+
+  // Functional casts in template functions
+  functional_cast_template_used_by_class(x);
+  functional_cast_template_used_by_int(x);
+
+  // New expressions are not functional casts
+  auto w = new int(x);
+
+  // Typedefs
+  S2 t = T(x); // OK, constructor call
+  S2 u = U(x); // NOK, it's a reinterpret_cast in disguise
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; use static_cast/const_cast/reinterpret_cast
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -133,9 +133,12 @@
 Changes in existing checks
 ^^
 
-- Removed default setting `cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"`,
+- Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
+- Updated :doc:`google-readability-casting
+  ` to diagnose and fix functional
+  casts, to achieve feature parity with the corresponding ``cpplint.py`` check.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -31,6 +31,11 @@
   unless(isInTemplateInstantiation()))
   .bind("cast"),
   this);
+  Finder->addMatcher(
+  cxxFunctionalCastExpr(unless(hasDescendant(cxxConstructExpr())),
+unless(hasDescendant(initListExpr(
+  .bind("cast"),
+  this);
 }
 
 static bool needsConstCast(QualType SourceType, QualType DestType) {
@@ -55,8 +60,39 @@
   return T1.getUnqualifiedType() == T2.getUnqualifiedType();
 }
 
+static clang::CharSourceRange getReplaceRange(const ExplicitCastExpr *Expr) {
+  if (const auto *CastExpr = dyn_cast(Expr)) {
+return CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  } else if (const auto *CastExpr = dyn_cast(Expr)) {
+return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
+ CastExpr->getLParenLoc());
+  } else
+llvm_unreachable("Unsupported CastExpr");
+}
+
+static StringRef getDestTypeString(const SourceManager ,
+   const LangOptions ,
+   const ExplicitCastExpr *Expr) {
+  SourceLocation BeginLoc;
+  SourceLocation EndLoc;
+
+  if (const auto *CastExpr = 

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks a lot for the review! I've fixed the last comment by @salman-javed-nz . 
Since that was the last standing comment I'll push :)


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 390580.
carlosgalvezp added a comment.

Fixed doc.


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

https://reviews.llvm.org/D114427

Files:
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
@@ -143,11 +143,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant cast to the same type
   // CHECK-FIXES: {{^}}  kZero;
 
-  int b2 = int(b);
-  int b3 = static_cast(b);
-  int b4 = b;
+  int b2 = static_cast(b);
+  int b3 = b;
   double aa = a;
-  (void)b2;
+  (void)aa;
   return (void)g();
 }
 
@@ -321,3 +320,50 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: C-style casts are discouraged; use constructor call syntax [
   // CHECK-FIXES: auto s6 = S(cr);
 }
+
+template 
+T functional_cast_template_used_by_class(float i) {
+  return T(i);
+}
+
+template 
+T functional_cast_template_used_by_int(float i) {
+  return T(i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: return static_cast(i);
+}
+
+struct S2 {
+  S2(float);
+};
+using T = S2;
+using U = S2 &;
+
+void functional_casts() {
+  float x = 1.5F;
+  auto y = int(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: auto y = static_cast(x);
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wc++11-narrowing"
+  // This if fine, compiler will warn about implicit conversions with brace initialization
+  auto z = int{x};
+#pragma clang diagnostic pop
+
+  // Functional casts are allowed if S is of class type
+  const char *str = "foo";
+  auto s = S(str);
+
+  // Functional casts in template functions
+  functional_cast_template_used_by_class(x);
+  functional_cast_template_used_by_int(x);
+
+  // New expressions are not functional casts
+  auto w = new int(x);
+
+  // Typedefs
+  S2 t = T(x); // OK, constructor call
+  S2 u = U(x); // NOK, it's a reinterpret_cast in disguise
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; use static_cast/const_cast/reinterpret_cast
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -133,9 +133,12 @@
 Changes in existing checks
 ^^
 
-- Removed default setting `cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"`,
+- Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
+- Updated :doc:`google-readability-casting
+  ` to diagnose and fix functional
+  casts, to achieve feature parity with the corresponding ``cpplint.py`` check.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -31,6 +31,11 @@
   unless(isInTemplateInstantiation()))
   .bind("cast"),
   this);
+  Finder->addMatcher(
+  cxxFunctionalCastExpr(unless(hasDescendant(cxxConstructExpr())),
+unless(hasDescendant(initListExpr(
+  .bind("cast"),
+  this);
 }
 
 static bool needsConstCast(QualType SourceType, QualType DestType) {
@@ -55,8 +60,39 @@
   return T1.getUnqualifiedType() == T2.getUnqualifiedType();
 }
 
+static clang::CharSourceRange getReplaceRange(const ExplicitCastExpr *Expr) {
+  if (const auto *CastExpr = dyn_cast(Expr)) {
+return CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  } else if (const auto *CastExpr = dyn_cast(Expr)) {
+return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
+ CastExpr->getLParenLoc());
+  } else
+llvm_unreachable("Unsupported CastExpr");
+}
+
+static StringRef getDestTypeString(const SourceManager ,
+   const LangOptions ,
+   const ExplicitCastExpr *Expr) {
+  SourceLocation BeginLoc;
+  SourceLocation EndLoc;
+
+  if (const auto *CastExpr = dyn_cast(Expr)) {
+BeginLoc = CastExpr->getLParenLoc().getLocWithOffset(1);
+EndLoc = CastExpr->getRParenLoc().getLocWithOffset(-1);
+  } else if (const auto *CastExpr = 

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

LGTM!




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:318
   // FIXME: This should be a static_cast.
   // C HECK-FIXES: auto s5 = static_cast(cr);
   auto s6 = (S)cr;

salman-javed-nz wrote:
> Isn't this intentional? Isn't it saying what the CHECK-MESSAGES should look 
> like in the future once the FIXME is done?
Ah, I guess that must be it.
(`C HECK-FIXES: ...` is a really subtle idiom for `FIXME the line above should 
become: ...`, but OK!)


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Nothing else that comes to mind that I want to see.
@Quuxplusone are you OK with the latest round of diffs?




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:318
   // FIXME: This should be a static_cast.
   // C HECK-FIXES: auto s5 = static_cast(cr);
   auto s6 = (S)cr;

Isn't this intentional? Isn't it saying what the CHECK-MESSAGES should look 
like in the future once the FIXME is done?


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:63-73
+static clang::CharSourceRange getReplaceRange(const ExplicitCastExpr *Expr) {
+  if (const auto *CastExpr = dyn_cast(Expr)) {
+return CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  } else if (const auto *CastExpr = dyn_cast(Expr)) {
+return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),

Looks much better than my suggested change. Good stuff.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:139
 
+- Updated ``google-readability-casting`` to diagnose and fix functional casts, 
to achieve feature
+  parity with the corresponding ``cpplint.py`` check.

Should this be like the
```
:doc:`bugprone-suspicious-memory-comparison
  `
```
a few lines above.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 390394.
carlosgalvezp added a comment.

Move logic into single functions.


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

https://reviews.llvm.org/D114427

Files:
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
@@ -143,11 +143,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant cast to the same type
   // CHECK-FIXES: {{^}}  kZero;
 
-  int b2 = int(b);
-  int b3 = static_cast(b);
-  int b4 = b;
+  int b2 = static_cast(b);
+  int b3 = b;
   double aa = a;
-  (void)b2;
+  (void)aa;
   return (void)g();
 }
 
@@ -321,3 +320,50 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: C-style casts are discouraged; use constructor call syntax [
   // CHECK-FIXES: auto s6 = S(cr);
 }
+
+template 
+T functional_cast_template_used_by_class(float i) {
+  return T(i);
+}
+
+template 
+T functional_cast_template_used_by_int(float i) {
+  return T(i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: return static_cast(i);
+}
+
+struct S2 {
+  S2(float);
+};
+using T = S2;
+using U = S2 &;
+
+void functional_casts() {
+  float x = 1.5F;
+  auto y = int(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: auto y = static_cast(x);
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wc++11-narrowing"
+  // This if fine, compiler will warn about implicit conversions with brace initialization
+  auto z = int{x};
+#pragma clang diagnostic pop
+
+  // Functional casts are allowed if S is of class type
+  const char *str = "foo";
+  auto s = S(str);
+
+  // Functional casts in template functions
+  functional_cast_template_used_by_class(x);
+  functional_cast_template_used_by_int(x);
+
+  // New expressions are not functional casts
+  auto w = new int(x);
+
+  // Typedefs
+  S2 t = T(x); // OK, constructor call
+  S2 u = U(x); // NOK, it's a reinterpret_cast in disguise
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; use static_cast/const_cast/reinterpret_cast
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -133,9 +133,11 @@
 Changes in existing checks
 ^^
 
-- Removed default setting `cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"`,
+- Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
+- Updated ``google-readability-casting`` to diagnose and fix functional casts, to achieve feature
+  parity with the corresponding ``cpplint.py`` check.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -31,6 +31,11 @@
   unless(isInTemplateInstantiation()))
   .bind("cast"),
   this);
+  Finder->addMatcher(
+  cxxFunctionalCastExpr(unless(hasDescendant(cxxConstructExpr())),
+unless(hasDescendant(initListExpr(
+  .bind("cast"),
+  this);
 }
 
 static bool needsConstCast(QualType SourceType, QualType DestType) {
@@ -55,8 +60,39 @@
   return T1.getUnqualifiedType() == T2.getUnqualifiedType();
 }
 
+static clang::CharSourceRange getReplaceRange(const ExplicitCastExpr *Expr) {
+  if (const auto *CastExpr = dyn_cast(Expr)) {
+return CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  } else if (const auto *CastExpr = dyn_cast(Expr)) {
+return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
+ CastExpr->getLParenLoc());
+  } else
+llvm_unreachable("Unsupported CastExpr");
+}
+
+static StringRef getDestTypeString(const SourceManager ,
+   const LangOptions ,
+   const ExplicitCastExpr *Expr) {
+  SourceLocation BeginLoc;
+  SourceLocation EndLoc;
+
+  if (const auto *CastExpr = dyn_cast(Expr)) {
+BeginLoc = CastExpr->getLParenLoc().getLocWithOffset(1);
+EndLoc = CastExpr->getRParenLoc().getLocWithOffset(-1);
+  } else if (const auto *CastExpr 

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:63-79
+static clang::CharSourceRange getReplaceRange(const CStyleCastExpr *CastExpr) {
+  return CharSourceRange::getCharRange(
+  CastExpr->getLParenLoc(), 
CastExpr->getSubExprAsWritten()->getBeginLoc());
+}
+
+static clang::CharSourceRange
+getReplaceRange(const CXXFunctionalCastExpr *CastExpr) {

Ditto here:
```
static clang::CharSourceRange
getReplaceRange(const ExplicitCastExpr *Expr) {
  if (const auto *CastExpr = dyn_cast(Expr)) {
return CharSourceRange::getCharRange(
CastExpr->getLParenLoc(), 
CastExpr->getSubExprAsWritten()->getBeginLoc());
  } else if (const auto *CastExpr = dyn_cast(Expr)) {
return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
 CastExpr->getLParenLoc());
  } else {
// however Clang spells "unreachable"
  }
}
```
Besides saving some reader-brain-cells, this also makes it clearer that there's 
a potential null dereference in the old code (if both `dyn_cast`s fail) that 
we're hoping is unreachable. This way, we have a place we can annotate that 
with `assert(false)` or whatever, instead of just dereferencing null.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:101-110
+static StringRef getDestTypeString(const SourceManager ,
+   const LangOptions ,
+   const ExplicitCastExpr *CastExpr) {
+  return isa(CastExpr)
+ ? getDestTypeString(SM, LangOpts,
+ dyn_cast(CastExpr))
+ : getDestTypeString(

I actually meant to move the conditional //inside// the body, like this. ^
Now there's only one function, and the reader doesn't have to do mental 
overload resolution to figure out what the caller's behavior is intended to be.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:318
   // FIXME: This should be a static_cast.
   // C HECK-FIXES: auto s5 = static_cast(cr);
   auto s6 = (S)cr;

Pre-existing: This looks accidental.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D114427#3158405 , @Quuxplusone 
wrote:

> Marking "accepted" for the record; but my checkmark means merely "I'm not 
> intending to block this," not "I claim the authority to say you //should// 
> land this." :)

Thanks! I was recently told that this is not recommended, as the patch no 
longer shows the status "Needs review to proceed" and reviewers might not be 
able to see it immediately in their dashboards, thus delaying review.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:170
+  : getDestTypeString(SM, getLangOpts(),
+  dyn_cast(CastExpr));
 

Quuxplusone wrote:
> IMO, this repeated conditional (compare lines 119–122) should be factored out 
> into the //body// of the helper function `getDestTypeString` (rather than 
> being repeated at every call-site), and `getDestTypeString` should take a 
> `const ExplicitCastExpr *` instead of having two overloads. (Notice that you 
> never //use// the overloading for anything: everywhere you call into the 
> overload set, you do so with a non-dependent `dyn_cast` wrapped in a `?:`, 
> indicating that you don't really want overloading at all.)
> ```
> StringRef DestTypeString = getDestTypeString(SM, getLangOpts(), CastExpr);
> ```
Updated to move the conditional into the function. I cannot avoid the casting 
though, because there is no common base class of `CXXFunctionalCastExpr` and 
`CStyleCastExpr` that has the methods `getLParenLoc` and so on, so that's why I 
need the type explicitly instead of invoking those methods from the base class.

The original solution used templates instead of overloading for this, but 
@salman-javed-nz suggested overloading instead. I think that's a bit easier to 
read IMO, with small functions having a single responsibility.

Let me know if you like the update :) 



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:358-360
+  // Functional casts in template functions
+  functional_cast_template_used_by_class(x);
+  functional_cast_template_used_by_int(x);

Quuxplusone wrote:
> FWIW, I'd prefer to instantiate the same function template in both cases 
> (because that's the interesting case for practical purposes — a template 
> that's only instantiated once doesn't pose a problem for the programmer). But 
> I get that you're doing this because it's easier to express the expected 
> output.
Yeah I'd prefer that too, but I wasn't sure how to set expectations on the same 
line for different inputs.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 390380.
carlosgalvezp marked 6 inline comments as done.
carlosgalvezp added a comment.

Moved conditionals inside function body.


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

https://reviews.llvm.org/D114427

Files:
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
@@ -143,11 +143,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant cast to the same type
   // CHECK-FIXES: {{^}}  kZero;
 
-  int b2 = int(b);
-  int b3 = static_cast(b);
-  int b4 = b;
+  int b2 = static_cast(b);
+  int b3 = b;
   double aa = a;
-  (void)b2;
+  (void)aa;
   return (void)g();
 }
 
@@ -321,3 +320,50 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: C-style casts are discouraged; use constructor call syntax [
   // CHECK-FIXES: auto s6 = S(cr);
 }
+
+template 
+T functional_cast_template_used_by_class(float i) {
+  return T(i);
+}
+
+template 
+T functional_cast_template_used_by_int(float i) {
+  return T(i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: return static_cast(i);
+}
+
+struct S2 {
+  S2(float);
+};
+using T = S2;
+using U = S2 &;
+
+void functional_casts() {
+  float x = 1.5F;
+  auto y = int(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: auto y = static_cast(x);
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wc++11-narrowing"
+  // This if fine, compiler will warn about implicit conversions with brace initialization
+  auto z = int{x};
+#pragma clang diagnostic pop
+
+  // Functional casts are allowed if S is of class type
+  const char *str = "foo";
+  auto s = S(str);
+
+  // Functional casts in template functions
+  functional_cast_template_used_by_class(x);
+  functional_cast_template_used_by_int(x);
+
+  // New expressions are not functional casts
+  auto w = new int(x);
+
+  // Typedefs
+  S2 t = T(x); // OK, constructor call
+  S2 u = U(x); // NOK, it's a reinterpret_cast in disguise
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; use static_cast/const_cast/reinterpret_cast
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -133,9 +133,11 @@
 Changes in existing checks
 ^^
 
-- Removed default setting `cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"`,
+- Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
+- Updated ``google-readability-casting`` to diagnose and fix functional casts, to achieve feature
+  parity with the corresponding ``cpplint.py`` check.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -31,6 +31,11 @@
   unless(isInTemplateInstantiation()))
   .bind("cast"),
   this);
+  Finder->addMatcher(
+  cxxFunctionalCastExpr(unless(hasDescendant(cxxConstructExpr())),
+unless(hasDescendant(initListExpr(
+  .bind("cast"),
+  this);
 }
 
 static bool needsConstCast(QualType SourceType, QualType DestType) {
@@ -55,8 +60,57 @@
   return T1.getUnqualifiedType() == T2.getUnqualifiedType();
 }
 
+static clang::CharSourceRange getReplaceRange(const CStyleCastExpr *CastExpr) {
+  return CharSourceRange::getCharRange(
+  CastExpr->getLParenLoc(), CastExpr->getSubExprAsWritten()->getBeginLoc());
+}
+
+static clang::CharSourceRange
+getReplaceRange(const CXXFunctionalCastExpr *CastExpr) {
+  return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
+   CastExpr->getLParenLoc());
+}
+
+static clang::CharSourceRange
+getReplaceRange(const ExplicitCastExpr *CastExpr) {
+  return isa(CastExpr)
+ ? getReplaceRange(dyn_cast(CastExpr))
+ : getReplaceRange(dyn_cast(CastExpr));
+}
+
+static StringRef getDestTypeString(const SourceManager ,
+   const LangOptions ,
+   const CStyleCastExpr *CastExpr) {
+  return Lexer::getSourceText(
+  

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.

Marking "accepted" for the record; but my checkmark means merely "I'm not 
intending to block this," not "I claim the authority to say you //should// land 
this." :)




Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:170
+  : getDestTypeString(SM, getLangOpts(),
+  dyn_cast(CastExpr));
 

IMO, this repeated conditional (compare lines 119–122) should be factored out 
into the //body// of the helper function `getDestTypeString` (rather than being 
repeated at every call-site), and `getDestTypeString` should take a `const 
ExplicitCastExpr *` instead of having two overloads. (Notice that you never 
//use// the overloading for anything: everywhere you call into the overload 
set, you do so with a non-dependent `dyn_cast` wrapped in a `?:`, indicating 
that you don't really want overloading at all.)
```
StringRef DestTypeString = getDestTypeString(SM, getLangOpts(), CastExpr);
```



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:358-360
+  // Functional casts in template functions
+  functional_cast_template_used_by_class(x);
+  functional_cast_template_used_by_int(x);

FWIW, I'd prefer to instantiate the same function template in both cases 
(because that's the interesting case for practical purposes — a template that's 
only instantiated once doesn't pose a problem for the programmer). But I get 
that you're doing this because it's easier to express the expected output.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:369
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; 
use static_cast/const_cast/reinterpret_cast
+}

It has also just now occurred to me that it might be interesting to see what 
happens with this warning in a SFINAE context, e.g.
```
template auto is_castable(int i) -> decltype(T(i), void(), true) { 
return true; }
```
but I suppose none of the things we could do there would be remotely sensible, 
so you'd just be testing basically that clang-tidy doesn't crash in that case. 
Anyway, no action needed.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342
+  auto w = new int(x);
+}

Quuxplusone wrote:
> salman-javed-nz wrote:
> > carlosgalvezp wrote:
> > > carlosgalvezp wrote:
> > > > Quuxplusone wrote:
> > > > > What about
> > > > > ```
> > > > > template T foo(int i) { return T(i); }
> > > > > int main() {
> > > > > foo>(); // valid, OK(!)
> > > > > foo(); // valid, not OK
> > > > > }
> > > > > ```
> > > > > What about
> > > > > ```
> > > > > struct Widget { Widget(int); };
> > > > > using T = Widget;
> > > > > using U = Widget&;
> > > > > int i = 42;
> > > > > Widget t = T(i);  // valid, OK?
> > > > > Widget u = U(i);  // valid C++, should definitely not be OK
> > > > > ```
> > > > > https://quuxplusone.github.io/blog/2020/01/22/expression-list-in-functional-cast/
> > > > Good point, thanks! I've added the first one to the unit test.
> > > > 
> > > > Regarding the second check, I'm not sure if it's the scope of this 
> > > > check. This check does not care whether the constructor of the class is 
> > > > implicit or not - if you use a class type, then you are calling the 
> > > > constructor so it's fine. Same goes when it's a reference - in my 
> > > > opinion this check is not concerned with that.
> > > > 
> > > > I definitely see the problems that can arise from the example that you 
> > > > posted, but maybe it fits better as a separate check in the `bugprone` 
> > > > category? This check (`google-readability-casting`) is focused only 
> > > > about avoiding C-style casting, i.e. it's a readability/style/modernize 
> > > > matter IMO. If cpplint is not diagnosing that, I don't think this check 
> > > > should either.
> > > It seems I should be able to just add the second example as a test and 
> > > clang-tidy would warn but, what should be the fixit for it? A 
> > > `static_cast` would lead to compiler error (which I personally would 
> > > gladly take, but I don't know in general if we want clang-tidy to "fix" 
> > > code leading to compiler errors"). Adding an ad-hoc message for this 
> > > particular error seems out of the scope of a "readability" check. 
> > > 
> > > What do you guys think?
> > > It seems I should be able to just add the second example as a test and 
> > > clang-tidy would warn but, what should be the fixit for it?
> > 
> > If I run the second example, but with old style C casts instead:
> > 
> > Input:
> > ```lang=cpp
> > struct Widget { Widget(int); };
> > using T = Widget;
> > using U = Widget&;
> > int i = 42;
> > Widget t = (T)(i);
> > Widget u = (U)(i);
> > ```
> > 
> > Output after fixits:
> > ```lang=cpp
> > struct Widget { Widget(int); };
> > using T = Widget;
> > using U = Widget&;
> > int i = 42;
> > Widget t = T(i);
> > Widget u = (U)(i);
> > ```
> > 
> > I guess the fix `Widget t = T(i);` is OK as it is covered by this exception:
> > >You may use cast formats like `T(x)` only when `T` is a class type.
> > 
> > For the `Widget u = (U)(i);` line, clang-tidy has warned about it but not 
> > offered a fix.
> > What would be the right fixit for that anyway?
> > `Widget u = U(i);   -->   Widget u = static_cast(i);` ?
> 
> No, this is a reinterpret_cast, so it would be
> ```
> Widget u = reinterpret_cast(i);
> ```
> at least in C++. I don't know about C, but I imagine the problem doesn't come 
> up.
> 
> (If the programmer looks at this line and says "oh geez, that's wrong," well, 
> he'll either fix it or file a task to revisit weird reinterpret_casts in the 
> codebase. If the programmer thinks the cast is //correct//, then personally 
> I'd hope he rewrites it as `Widget u = *reinterpret_cast();` for 
> clarity, but that's not a clang-tidy issue.)
> 
> Relevant: "fixits versus suppression mechanisms" 
> https://quuxplusone.github.io/blog/2020/09/02/wparentheses/ 
> `reinterpret_cast` is a suppression mechanism; I infer that you're casting 
> about for a fixit, which won't exist in this case.
Added test case, currently it provides a generic comment.

Thanks a lot for the explanation, this was eye-opening to me. I only thought of 
reinterpret casts when using pointers, but of course references are kind of the 
same thing :) 

Let me know if you are happy with the patch!


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 390352.
carlosgalvezp added a comment.

Added Widget test case (reusing the S2 struct instead of adding a new struct 
Widget)


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

https://reviews.llvm.org/D114427

Files:
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
@@ -143,11 +143,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant cast to the same type
   // CHECK-FIXES: {{^}}  kZero;
 
-  int b2 = int(b);
-  int b3 = static_cast(b);
-  int b4 = b;
+  int b2 = static_cast(b);
+  int b3 = b;
   double aa = a;
-  (void)b2;
+  (void)aa;
   return (void)g();
 }
 
@@ -321,3 +320,50 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: C-style casts are discouraged; use constructor call syntax [
   // CHECK-FIXES: auto s6 = S(cr);
 }
+
+template 
+T functional_cast_template_used_by_class(float i) {
+  return T(i);
+}
+
+template 
+T functional_cast_template_used_by_int(float i) {
+  return T(i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: return static_cast(i);
+}
+
+struct S2 {
+  S2(float);
+};
+using T = S2;
+using U = S2 &;
+
+void functional_casts() {
+  float x = 1.5F;
+  auto y = int(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: auto y = static_cast(x);
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wc++11-narrowing"
+  // This if fine, compiler will warn about implicit conversions with brace initialization
+  auto z = int{x};
+#pragma clang diagnostic pop
+
+  // Functional casts are allowed if S is of class type
+  const char *str = "foo";
+  auto s = S(str);
+
+  // Functional casts in template functions
+  functional_cast_template_used_by_class(x);
+  functional_cast_template_used_by_int(x);
+
+  // New expressions are not functional casts
+  auto w = new int(x);
+
+  // Typedefs
+  S2 t = T(x); // OK, constructor call
+  S2 u = U(x); // NOK, it's a reinterpret_cast in disguise
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; use static_cast/const_cast/reinterpret_cast
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -133,9 +133,11 @@
 Changes in existing checks
 ^^
 
-- Removed default setting `cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"`,
+- Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
+- Updated ``google-readability-casting`` to diagnose and fix functional casts, to achieve feature
+  parity with the corresponding ``cpplint.py`` check.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -31,6 +31,11 @@
   unless(isInTemplateInstantiation()))
   .bind("cast"),
   this);
+  Finder->addMatcher(
+  cxxFunctionalCastExpr(unless(hasDescendant(cxxConstructExpr())),
+unless(hasDescendant(initListExpr(
+  .bind("cast"),
+  this);
 }
 
 static bool needsConstCast(QualType SourceType, QualType DestType) {
@@ -55,8 +60,39 @@
   return T1.getUnqualifiedType() == T2.getUnqualifiedType();
 }
 
+static clang::CharSourceRange getReplaceRange(const CStyleCastExpr *CastExpr) {
+  return CharSourceRange::getCharRange(
+  CastExpr->getLParenLoc(), CastExpr->getSubExprAsWritten()->getBeginLoc());
+}
+
+static clang::CharSourceRange
+getReplaceRange(const CXXFunctionalCastExpr *CastExpr) {
+  return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
+   CastExpr->getLParenLoc());
+}
+
+static StringRef getDestTypeString(const SourceManager ,
+   const LangOptions ,
+   const CStyleCastExpr *CastExpr) {
+  return Lexer::getSourceText(
+  CharSourceRange::getTokenRange(
+  CastExpr->getLParenLoc().getLocWithOffset(1),
+  CastExpr->getRParenLoc().getLocWithOffset(-1)),
+  SM, LangOpts);
+}
+
+static StringRef getDestTypeString(const SourceManager ,
+

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342
+  auto w = new int(x);
+}

salman-javed-nz wrote:
> carlosgalvezp wrote:
> > carlosgalvezp wrote:
> > > Quuxplusone wrote:
> > > > What about
> > > > ```
> > > > template T foo(int i) { return T(i); }
> > > > int main() {
> > > > foo>(); // valid, OK(!)
> > > > foo(); // valid, not OK
> > > > }
> > > > ```
> > > > What about
> > > > ```
> > > > struct Widget { Widget(int); };
> > > > using T = Widget;
> > > > using U = Widget&;
> > > > int i = 42;
> > > > Widget t = T(i);  // valid, OK?
> > > > Widget u = U(i);  // valid C++, should definitely not be OK
> > > > ```
> > > > https://quuxplusone.github.io/blog/2020/01/22/expression-list-in-functional-cast/
> > > Good point, thanks! I've added the first one to the unit test.
> > > 
> > > Regarding the second check, I'm not sure if it's the scope of this check. 
> > > This check does not care whether the constructor of the class is implicit 
> > > or not - if you use a class type, then you are calling the constructor so 
> > > it's fine. Same goes when it's a reference - in my opinion this check is 
> > > not concerned with that.
> > > 
> > > I definitely see the problems that can arise from the example that you 
> > > posted, but maybe it fits better as a separate check in the `bugprone` 
> > > category? This check (`google-readability-casting`) is focused only about 
> > > avoiding C-style casting, i.e. it's a readability/style/modernize matter 
> > > IMO. If cpplint is not diagnosing that, I don't think this check should 
> > > either.
> > It seems I should be able to just add the second example as a test and 
> > clang-tidy would warn but, what should be the fixit for it? A 
> > `static_cast` would lead to compiler error (which I personally would 
> > gladly take, but I don't know in general if we want clang-tidy to "fix" 
> > code leading to compiler errors"). Adding an ad-hoc message for this 
> > particular error seems out of the scope of a "readability" check. 
> > 
> > What do you guys think?
> > It seems I should be able to just add the second example as a test and 
> > clang-tidy would warn but, what should be the fixit for it?
> 
> If I run the second example, but with old style C casts instead:
> 
> Input:
> ```lang=cpp
> struct Widget { Widget(int); };
> using T = Widget;
> using U = Widget&;
> int i = 42;
> Widget t = (T)(i);
> Widget u = (U)(i);
> ```
> 
> Output after fixits:
> ```lang=cpp
> struct Widget { Widget(int); };
> using T = Widget;
> using U = Widget&;
> int i = 42;
> Widget t = T(i);
> Widget u = (U)(i);
> ```
> 
> I guess the fix `Widget t = T(i);` is OK as it is covered by this exception:
> >You may use cast formats like `T(x)` only when `T` is a class type.
> 
> For the `Widget u = (U)(i);` line, clang-tidy has warned about it but not 
> offered a fix.
> What would be the right fixit for that anyway?
> `Widget u = U(i);   -->   Widget u = static_cast(i);` ?

No, this is a reinterpret_cast, so it would be
```
Widget u = reinterpret_cast(i);
```
at least in C++. I don't know about C, but I imagine the problem doesn't come 
up.

(If the programmer looks at this line and says "oh geez, that's wrong," well, 
he'll either fix it or file a task to revisit weird reinterpret_casts in the 
codebase. If the programmer thinks the cast is //correct//, then personally I'd 
hope he rewrites it as `Widget u = *reinterpret_cast();` for 
clarity, but that's not a clang-tidy issue.)

Relevant: "fixits versus suppression mechanisms" 
https://quuxplusone.github.io/blog/2020/09/02/wparentheses/ `reinterpret_cast` 
is a suppression mechanism; I infer that you're casting about for a fixit, 
which won't exist in this case.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

I think the primary goal is satisfied - in all cases the cast is identified and 
a warning is generated.

For the `Widget&` case, a warning is generated but no fixit is offered, but 
that isn't any worse than the existing C-style cast fixits.
It does sound like a case where offering no fix is better than offering a fix 
that makes things worse.

What would be the right fixit for that anyway?
`Widget u = U(i);   -->   Widget u = T(i);` ?


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342
+  auto w = new int(x);
+}

carlosgalvezp wrote:
> carlosgalvezp wrote:
> > Quuxplusone wrote:
> > > What about
> > > ```
> > > template T foo(int i) { return T(i); }
> > > int main() {
> > > foo>(); // valid, OK(!)
> > > foo(); // valid, not OK
> > > }
> > > ```
> > > What about
> > > ```
> > > struct Widget { Widget(int); };
> > > using T = Widget;
> > > using U = Widget&;
> > > int i = 42;
> > > Widget t = T(i);  // valid, OK?
> > > Widget u = U(i);  // valid C++, should definitely not be OK
> > > ```
> > > https://quuxplusone.github.io/blog/2020/01/22/expression-list-in-functional-cast/
> > Good point, thanks! I've added the first one to the unit test.
> > 
> > Regarding the second check, I'm not sure if it's the scope of this check. 
> > This check does not care whether the constructor of the class is implicit 
> > or not - if you use a class type, then you are calling the constructor so 
> > it's fine. Same goes when it's a reference - in my opinion this check is 
> > not concerned with that.
> > 
> > I definitely see the problems that can arise from the example that you 
> > posted, but maybe it fits better as a separate check in the `bugprone` 
> > category? This check (`google-readability-casting`) is focused only about 
> > avoiding C-style casting, i.e. it's a readability/style/modernize matter 
> > IMO. If cpplint is not diagnosing that, I don't think this check should 
> > either.
> It seems I should be able to just add the second example as a test and 
> clang-tidy would warn but, what should be the fixit for it? A 
> `static_cast` would lead to compiler error (which I personally would 
> gladly take, but I don't know in general if we want clang-tidy to "fix" code 
> leading to compiler errors"). Adding an ad-hoc message for this particular 
> error seems out of the scope of a "readability" check. 
> 
> What do you guys think?
> It seems I should be able to just add the second example as a test and 
> clang-tidy would warn but, what should be the fixit for it?

If I run the second example, but with old style C casts instead:

Input:
```lang=cpp
struct Widget { Widget(int); };
using T = Widget;
using U = Widget&;
int i = 42;
Widget t = (T)(i);
Widget u = (U)(i);
```

Output after fixits:
```lang=cpp
struct Widget { Widget(int); };
using T = Widget;
using U = Widget&;
int i = 42;
Widget t = T(i);
Widget u = (U)(i);
```

I guess the fix `Widget t = T(i);` is OK as it is covered by this exception:
>You may use cast formats like `T(x)` only when `T` is a class type.

For the `Widget u = (U)(i);` line, clang-tidy has warned about it but not 
offered a fix.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Personally I think it's best to merge this as is, allowing people to e.g. 
replace cpplint with clang-tidy (one static analyzer to rule them all!), and 
add more functionality in a separate, focused patch. Looking forward to hearing 
your thoughts :)


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342
+  auto w = new int(x);
+}

carlosgalvezp wrote:
> Quuxplusone wrote:
> > What about
> > ```
> > template T foo(int i) { return T(i); }
> > int main() {
> > foo>(); // valid, OK(!)
> > foo(); // valid, not OK
> > }
> > ```
> > What about
> > ```
> > struct Widget { Widget(int); };
> > using T = Widget;
> > using U = Widget&;
> > int i = 42;
> > Widget t = T(i);  // valid, OK?
> > Widget u = U(i);  // valid C++, should definitely not be OK
> > ```
> > https://quuxplusone.github.io/blog/2020/01/22/expression-list-in-functional-cast/
> Good point, thanks! I've added the first one to the unit test.
> 
> Regarding the second check, I'm not sure if it's the scope of this check. 
> This check does not care whether the constructor of the class is implicit or 
> not - if you use a class type, then you are calling the constructor so it's 
> fine. Same goes when it's a reference - in my opinion this check is not 
> concerned with that.
> 
> I definitely see the problems that can arise from the example that you 
> posted, but maybe it fits better as a separate check in the `bugprone` 
> category? This check (`google-readability-casting`) is focused only about 
> avoiding C-style casting, i.e. it's a readability/style/modernize matter IMO. 
> If cpplint is not diagnosing that, I don't think this check should either.
It seems I should be able to just add the second example as a test and 
clang-tidy would warn but, what should be the fixit for it? A `static_cast` 
would lead to compiler error (which I personally would gladly take, but I don't 
know in general if we want clang-tidy to "fix" code leading to compiler 
errors"). Adding an ad-hoc message for this particular error seems out of the 
scope of a "readability" check. 

What do you guys think?


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342
+  auto w = new int(x);
+}

Quuxplusone wrote:
> What about
> ```
> template T foo(int i) { return T(i); }
> int main() {
> foo>(); // valid, OK(!)
> foo(); // valid, not OK
> }
> ```
> What about
> ```
> struct Widget { Widget(int); };
> using T = Widget;
> using U = Widget&;
> int i = 42;
> Widget t = T(i);  // valid, OK?
> Widget u = U(i);  // valid C++, should definitely not be OK
> ```
> https://quuxplusone.github.io/blog/2020/01/22/expression-list-in-functional-cast/
Good point, thanks! I've added the first one to the unit test.

Regarding the second check, I'm not sure if it's the scope of this check. This 
check does not care whether the constructor of the class is implicit or not - 
if you use a class type, then you are calling the constructor so it's fine. 
Same goes when it's a reference - in my opinion this check is not concerned 
with that.

I definitely see the problems that can arise from the example that you posted, 
but maybe it fits better as a separate check in the `bugprone` category? This 
check (`google-readability-casting`) is focused only about avoiding C-style 
casting, i.e. it's a readability/style/modernize matter IMO. If cpplint is not 
diagnosing that, I don't think this check should either.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 390191.
carlosgalvezp added a comment.

Add extra test for template functions that use the functional cast. It should 
only warn when T is not a class type.


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

https://reviews.llvm.org/D114427

Files:
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
@@ -143,11 +143,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant cast to the same type
   // CHECK-FIXES: {{^}}  kZero;
 
-  int b2 = int(b);
-  int b3 = static_cast(b);
-  int b4 = b;
+  int b2 = static_cast(b);
+  int b3 = b;
   double aa = a;
-  (void)b2;
+  (void)aa;
   return (void)g();
 }
 
@@ -321,3 +320,43 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: C-style casts are discouraged; use constructor call syntax [
   // CHECK-FIXES: auto s6 = S(cr);
 }
+
+template 
+T functional_cast_template_used_by_class(float i) {
+  return T(i);
+}
+
+template 
+T functional_cast_template_used_by_int(float i) {
+  return T(i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: return static_cast(i);
+}
+
+struct S2 {
+  S2(float) {}
+};
+
+void functional_casts() {
+  float x = 1.5F;
+  auto y = int(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: auto y = static_cast(x);
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wc++11-narrowing"
+  // This if fine, compiler will warn about implicit conversions with brace initialization
+  auto z = int{x};
+#pragma clang diagnostic pop
+
+  // Functional casts are allowed if S is of class type
+  const char *str = "foo";
+  auto s = S(str);
+
+  // Functional casts in template functions
+  functional_cast_template_used_by_class(x);
+  functional_cast_template_used_by_int(x);
+
+  // New expressions are not functional casts
+  auto w = new int(x);
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -133,9 +133,11 @@
 Changes in existing checks
 ^^
 
-- Removed default setting `cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"`,
+- Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
+- Updated ``google-readability-casting`` to diagnose and fix functional casts, to achieve feature
+  parity with the corresponding ``cpplint.py`` check.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -31,6 +31,11 @@
   unless(isInTemplateInstantiation()))
   .bind("cast"),
   this);
+  Finder->addMatcher(
+  cxxFunctionalCastExpr(unless(hasDescendant(cxxConstructExpr())),
+unless(hasDescendant(initListExpr(
+  .bind("cast"),
+  this);
 }
 
 static bool needsConstCast(QualType SourceType, QualType DestType) {
@@ -55,8 +60,39 @@
   return T1.getUnqualifiedType() == T2.getUnqualifiedType();
 }
 
+static clang::CharSourceRange getReplaceRange(const CStyleCastExpr *CastExpr) {
+  return CharSourceRange::getCharRange(
+  CastExpr->getLParenLoc(), CastExpr->getSubExprAsWritten()->getBeginLoc());
+}
+
+static clang::CharSourceRange
+getReplaceRange(const CXXFunctionalCastExpr *CastExpr) {
+  return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
+   CastExpr->getLParenLoc());
+}
+
+static StringRef getDestTypeString(const SourceManager ,
+   const LangOptions ,
+   const CStyleCastExpr *CastExpr) {
+  return Lexer::getSourceText(
+  CharSourceRange::getTokenRange(
+  CastExpr->getLParenLoc().getLocWithOffset(1),
+  CastExpr->getRParenLoc().getLocWithOffset(-1)),
+  SM, LangOpts);
+}
+
+static StringRef getDestTypeString(const SourceManager ,
+   const LangOptions ,
+   const CXXFunctionalCastExpr *CastExpr) {
+  return Lexer::getSourceText(
+  CharSourceRange::getTokenRange(
+  CastExpr->getBeginLoc(),
+  

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342
+  auto w = new int(x);
+}

What about
```
template T foo(int i) { return T(i); }
int main() {
foo>(); // valid, OK(!)
foo(); // valid, not OK
}
```
What about
```
struct Widget { Widget(int); };
using T = Widget;
using U = Widget&;
int i = 42;
Widget t = T(i);  // valid, OK?
Widget u = U(i);  // valid C++, should definitely not be OK
```
https://quuxplusone.github.io/blog/2020/01/22/expression-list-in-functional-cast/


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz accepted this revision.
salman-javed-nz added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:335
+  const char *str = "foo";
+  auto s = S(str);
+}

carlosgalvezp wrote:
> salman-javed-nz wrote:
> > Is a test to check `new int(x)` worth including? I see that the cpplint 
> > guys explicitly filter it out of their results.
> Sure, even though I think technically it's not a cast. At least it's not 
> shown as such in the AST.
It's not a cast in the AST, but it's nice to document in the unit test that we 
have considered it and that that intend to treat it no differently to how 
cpplint treats it.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:122
+  ? getReplaceRange(dyn_cast(CastExpr))
+  : getReplaceRange(dyn_cast(CastExpr));
 

One problem I see here is in the future someone adds a 3rd class of casts - 
then this should become an if-else. That's why I had it like that before, but 
perhaps it's too "defensive" at this point.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:99-106
+  CharSourceRange ReplaceRange;
+  if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  else if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(CastExpr->getBeginLoc(),

salman-javed-nz wrote:
> The majority of `checkExpr()`'s contents are common to both types, 
> `CStyleCastExpr` and `CXXFunctionalCastExpr`.
> Only the `ReplaceRange = CharSourceRange::getCharRange...` and the 
> `DestTypeString = Lexer::getSourceText...` parts change depending on the Expr 
> type.
> 
> What about breaking those two assignments out into their own functions, 
> rather than templating the entire `checkExpr()` function?
> 
> For example, (note: untested code)
> 
> ```lang=cpp
> clang::CharSourceRange GetReplaceRange(const CStyleCastExpr *CastExpr) {
>   return CharSourceRange::getCharRange(
>   CastExpr->getLParenLoc(), 
> CastExpr->getSubExprAsWritten()->getBeginLoc());
> }
> 
> clang::CharSourceRange GetReplaceRange(const CXXFunctionalCastExpr *CastExpr) 
> {
>   return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
>CastExpr->getLParenLoc());
> }
> 
> ...
> 
> CharSourceRange ReplaceRange =
>   isa(CastExpr)
>   ? GetReplaceRange(dyn_cast(CastExpr))
>   : GetReplaceRange(dyn_cast(CastExpr));
> ```
> 
> Would something like that work?
Thanks for the suggestion, much cleaner! I've made `CastExpr` become a 
`ExplicitCastExpr` instead (which is common base to both cast classes) to be 
able to handle only one object.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:335
+  const char *str = "foo";
+  auto s = S(str);
+}

salman-javed-nz wrote:
> Is a test to check `new int(x)` worth including? I see that the cpplint guys 
> explicitly filter it out of their results.
Sure, even though I think technically it's not a cast. At least it's not shown 
as such in the AST.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 390165.
carlosgalvezp marked 5 inline comments as done.
carlosgalvezp added a comment.

Addressed comments.


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

https://reviews.llvm.org/D114427

Files:
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
@@ -143,11 +143,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant cast to the same type
   // CHECK-FIXES: {{^}}  kZero;
 
-  int b2 = int(b);
-  int b3 = static_cast(b);
-  int b4 = b;
+  int b2 = static_cast(b);
+  int b3 = b;
   double aa = a;
-  (void)b2;
+  (void)aa;
   return (void)g();
 }
 
@@ -321,3 +320,23 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: C-style casts are discouraged; use constructor call syntax [
   // CHECK-FIXES: auto s6 = S(cr);
 }
+
+void functional_casts() {
+  float x = 1.5F;
+  auto y = int(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: auto y = static_cast(x);
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wc++11-narrowing"
+  // This if fine, compiler will warn about implicit conversions with brace initialization
+  auto z = int{x};
+#pragma clang diagnostic pop
+
+  // Functional casts are allowed if S is of class type
+  const char *str = "foo";
+  auto s = S(str);
+
+  // New expressions are not functional casts
+  auto w = new int(x);
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -133,9 +133,11 @@
 Changes in existing checks
 ^^
 
-- Removed default setting `cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"`,
+- Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
+- Updated ``google-readability-casting`` to diagnose and fix functional casts, to achieve feature
+  parity with the corresponding ``cpplint.py`` check.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -31,6 +31,11 @@
   unless(isInTemplateInstantiation()))
   .bind("cast"),
   this);
+  Finder->addMatcher(
+  cxxFunctionalCastExpr(unless(hasDescendant(cxxConstructExpr())),
+unless(hasDescendant(initListExpr(
+  .bind("cast"),
+  this);
 }
 
 static bool needsConstCast(QualType SourceType, QualType DestType) {
@@ -55,8 +60,39 @@
   return T1.getUnqualifiedType() == T2.getUnqualifiedType();
 }
 
+static clang::CharSourceRange getReplaceRange(const CStyleCastExpr *CastExpr) {
+  return CharSourceRange::getCharRange(
+  CastExpr->getLParenLoc(), CastExpr->getSubExprAsWritten()->getBeginLoc());
+}
+
+static clang::CharSourceRange
+getReplaceRange(const CXXFunctionalCastExpr *CastExpr) {
+  return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
+   CastExpr->getLParenLoc());
+}
+
+static StringRef getDestTypeString(const SourceManager ,
+   const LangOptions ,
+   const CStyleCastExpr *CastExpr) {
+  return Lexer::getSourceText(
+  CharSourceRange::getTokenRange(
+  CastExpr->getLParenLoc().getLocWithOffset(1),
+  CastExpr->getRParenLoc().getLocWithOffset(-1)),
+  SM, LangOpts);
+}
+
+static StringRef getDestTypeString(const SourceManager ,
+   const LangOptions ,
+   const CXXFunctionalCastExpr *CastExpr) {
+  return Lexer::getSourceText(
+  CharSourceRange::getTokenRange(
+  CastExpr->getBeginLoc(),
+  CastExpr->getLParenLoc().getLocWithOffset(-1)),
+  SM, LangOpts);
+}
+
 void AvoidCStyleCastsCheck::check(const MatchFinder::MatchResult ) {
-  const auto *CastExpr = Result.Nodes.getNodeAs("cast");
+  const auto *CastExpr = Result.Nodes.getNodeAs("cast");
 
   // Ignore casts in macros.
   if (CastExpr->getExprLoc().isMacroID())
@@ -80,8 +116,10 @@
   const QualType SourceType = SourceTypeAsWritten.getCanonicalType();
   const QualType DestType = DestTypeAsWritten.getCanonicalType();
 
-  auto ReplaceRange = 

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:100-104
+  if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  else if (isa(CastExpr))

Should this be an `if ... else`?


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Thanks for the patch.

Just a little bit of feedback but overall I'm happy with the approach taken.




Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:99-106
+  CharSourceRange ReplaceRange;
+  if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  else if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(CastExpr->getBeginLoc(),

The majority of `checkExpr()`'s contents are common to both types, 
`CStyleCastExpr` and `CXXFunctionalCastExpr`.
Only the `ReplaceRange = CharSourceRange::getCharRange...` and the 
`DestTypeString = Lexer::getSourceText...` parts change depending on the Expr 
type.

What about breaking those two assignments out into their own functions, rather 
than templating the entire `checkExpr()` function?

For example, (note: untested code)

```lang=cpp
clang::CharSourceRange GetReplaceRange(const CStyleCastExpr *CastExpr) {
  return CharSourceRange::getCharRange(
  CastExpr->getLParenLoc(), CastExpr->getSubExprAsWritten()->getBeginLoc());
}

clang::CharSourceRange GetReplaceRange(const CXXFunctionalCastExpr *CastExpr) {
  return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
   CastExpr->getLParenLoc());
}

...

CharSourceRange ReplaceRange =
  isa(CastExpr)
  ? GetReplaceRange(dyn_cast(CastExpr))
  : GetReplaceRange(dyn_cast(CastExpr));
```

Would something like that work?



Comment at: 
clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:151-162
+  if (isa(CastExpr))
+DestTypeString =
+Lexer::getSourceText(CharSourceRange::getTokenRange(
+ CastExpr->getLParenLoc().getLocWithOffset(1),
+ 
CastExpr->getRParenLoc().getLocWithOffset(-1)),
+ SM, getLangOpts());
+  else if (isa(CastExpr))

See comment above.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:139-140
 
+- Updated `google-readability-casting` to diagnose and fix functional casts, 
to achieve feature
+  parity with the corresponding `cpplint.py` check.
 

Single back-ticks are used to do linking. Double back-ticks is probably what 
you're after.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:335
+  const char *str = "foo";
+  auto s = S(str);
+}

Is a test to check `new int(x)` worth including? I see that the cpplint guys 
explicitly filter it out of their results.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 389159.
carlosgalvezp added a comment.

Fix numbering in variables.


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

https://reviews.llvm.org/D114427

Files:
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11-or-later %s google-readability-casting %t
+// RUN: %check_clang_tidy -std=c++11-or-later %s google-readability-casting %t -- -- -Wno-c++11-narrowing
 
 bool g() { return false; }
 
@@ -143,11 +143,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant cast to the same type
   // CHECK-FIXES: {{^}}  kZero;
 
-  int b2 = int(b);
-  int b3 = static_cast(b);
-  int b4 = b;
+  int b2 = static_cast(b);
+  int b3 = b;
   double aa = a;
-  (void)b2;
+  (void)aa;
   return (void)g();
 }
 
@@ -321,3 +320,17 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: C-style casts are discouraged; use constructor call syntax [
   // CHECK-FIXES: auto s6 = S(cr);
 }
+
+void functional_casts() {
+  float x = 1.5F;
+  auto y = int(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: auto y = static_cast(x);
+
+  // This if fine, compiler won't allow implicit conversions with brace initialization
+  auto z = int{x};
+
+  // Functional casts are allowed if S is of class type
+  const char *str = "foo";
+  auto s = S(str);
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,6 +136,8 @@
 - Removed default setting `cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"`,
   to match the current state of the C++ Core Guidelines.
 
+- Updated `google-readability-casting` to diagnose and fix functional casts, to achieve feature
+  parity with the corresponding `cpplint.py` check.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
@@ -34,6 +34,11 @@
   : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
+
+private:
+  template 
+  void checkExpr(const ast_matchers::MatchFinder::MatchResult ,
+ const CastExprT *CastExpr);
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -31,6 +31,11 @@
   unless(isInTemplateInstantiation()))
   .bind("cast"),
   this);
+  Finder->addMatcher(
+  cxxFunctionalCastExpr(unless(hasDescendant(cxxConstructExpr())),
+unless(hasDescendant(initListExpr(
+  .bind("functional_cast"),
+  this);
 }
 
 static bool needsConstCast(QualType SourceType, QualType DestType) {
@@ -57,7 +62,18 @@
 
 void AvoidCStyleCastsCheck::check(const MatchFinder::MatchResult ) {
   const auto *CastExpr = Result.Nodes.getNodeAs("cast");
+  const auto *FunctionalCastExpr =
+  Result.Nodes.getNodeAs("functional_cast");
+
+  if (CastExpr)
+checkExpr(Result, CastExpr);
+  else if (FunctionalCastExpr)
+checkExpr(Result, FunctionalCastExpr);
+}
 
+template 
+void AvoidCStyleCastsCheck::checkExpr(const MatchFinder::MatchResult ,
+  const CastExprT *CastExpr) {
   // Ignore casts in macros.
   if (CastExpr->getExprLoc().isMacroID())
 return;
@@ -80,8 +96,14 @@
   const QualType SourceType = SourceTypeAsWritten.getCanonicalType();
   const QualType DestType = DestTypeAsWritten.getCanonicalType();
 
-  auto ReplaceRange = CharSourceRange::getCharRange(
-  CastExpr->getLParenLoc(), CastExpr->getSubExprAsWritten()->getBeginLoc());
+  CharSourceRange ReplaceRange;
+  if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  else if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
+

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 389157.
carlosgalvezp added a comment.

Fix numbering of variables.


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

https://reviews.llvm.org/D114427

Files:
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11-or-later %s google-readability-casting %t
+// RUN: %check_clang_tidy -std=c++11-or-later %s google-readability-casting %t -- -- -Wno-c++11-narrowing
 
 bool g() { return false; }
 
@@ -143,11 +143,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant cast to the same type
   // CHECK-FIXES: {{^}}  kZero;
 
-  int b2 = int(b);
   int b3 = static_cast(b);
   int b4 = b;
   double aa = a;
-  (void)b2;
+  (void)aa;
   return (void)g();
 }
 
@@ -321,3 +320,17 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: C-style casts are discouraged; use constructor call syntax [
   // CHECK-FIXES: auto s6 = S(cr);
 }
+
+void functional_casts() {
+  float x = 1.5F;
+  auto y = int(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: auto y = static_cast(x);
+
+  // This if fine, compiler won't allow implicit conversions with brace initialization
+  auto z = int{x};
+
+  // Functional casts are allowed if S is of class type
+  const char *str = "foo";
+  auto s = S(str);
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,6 +136,8 @@
 - Removed default setting `cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"`,
   to match the current state of the C++ Core Guidelines.
 
+- Updated `google-readability-casting` to diagnose and fix functional casts, to achieve feature
+  parity with the corresponding `cpplint.py` check.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
@@ -34,6 +34,11 @@
   : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
+
+private:
+  template 
+  void checkExpr(const ast_matchers::MatchFinder::MatchResult ,
+ const CastExprT *CastExpr);
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -31,6 +31,11 @@
   unless(isInTemplateInstantiation()))
   .bind("cast"),
   this);
+  Finder->addMatcher(
+  cxxFunctionalCastExpr(unless(hasDescendant(cxxConstructExpr())),
+unless(hasDescendant(initListExpr(
+  .bind("functional_cast"),
+  this);
 }
 
 static bool needsConstCast(QualType SourceType, QualType DestType) {
@@ -57,7 +62,18 @@
 
 void AvoidCStyleCastsCheck::check(const MatchFinder::MatchResult ) {
   const auto *CastExpr = Result.Nodes.getNodeAs("cast");
+  const auto *FunctionalCastExpr =
+  Result.Nodes.getNodeAs("functional_cast");
+
+  if (CastExpr)
+checkExpr(Result, CastExpr);
+  else if (FunctionalCastExpr)
+checkExpr(Result, FunctionalCastExpr);
+}
 
+template 
+void AvoidCStyleCastsCheck::checkExpr(const MatchFinder::MatchResult ,
+  const CastExprT *CastExpr) {
   // Ignore casts in macros.
   if (CastExpr->getExprLoc().isMacroID())
 return;
@@ -80,8 +96,14 @@
   const QualType SourceType = SourceTypeAsWritten.getCanonicalType();
   const QualType DestType = DestTypeAsWritten.getCanonicalType();
 
-  auto ReplaceRange = CharSourceRange::getCharRange(
-  CastExpr->getLParenLoc(), CastExpr->getSubExprAsWritten()->getBeginLoc());
+  CharSourceRange ReplaceRange;
+  if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  else if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
+ 

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
Herald added a subscriber: xazax.hun.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

The google-readability-casting check is meant to be on par
with cpplint's readability/casting check, according to the
documentation. However it currently does not diagnose
functional casts, like:

float x = 1.5F;
int y = int(x);

This is detected by cpplint, however, and the guidelines
are clear that such a cast is only allowed when the type
is a class type (constructor call):

> You may use cast formats like `T(x)` only when `T` is a class type.

Therefore, update the clang-tidy check to check this
case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114427

Files:
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
  clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11-or-later %s google-readability-casting %t
+// RUN: %check_clang_tidy -std=c++11-or-later %s google-readability-casting %t -- -- -Wno-c++11-narrowing
 
 bool g() { return false; }
 
@@ -143,11 +143,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant cast to the same type
   // CHECK-FIXES: {{^}}  kZero;
 
-  int b2 = int(b);
   int b3 = static_cast(b);
   int b4 = b;
   double aa = a;
-  (void)b2;
+  (void)aa;
   return (void)g();
 }
 
@@ -321,3 +320,17 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: C-style casts are discouraged; use constructor call syntax [
   // CHECK-FIXES: auto s6 = S(cr);
 }
+
+void functional_casts() {
+  float x = 1.5F;
+  auto y = int(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C-style casts are discouraged; use static_cast
+  // CHECK-FIXES: auto y = static_cast(x);
+
+  // This if fine, compiler won't allow implicit conversions with brace initialization
+  auto z = int{x};
+
+  // Functional casts are allowed if S is of class type
+  const char *str = "foo";
+  auto s = S(str);
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,6 +136,8 @@
 - Removed default setting `cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"`,
   to match the current state of the C++ Core Guidelines.
 
+- Updated `google-readability-casting` to diagnose and fix functional casts, to achieve feature
+  parity with the corresponding `cpplint.py` check.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.h
@@ -34,6 +34,11 @@
   : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
+
+private:
+  template 
+  void checkExpr(const ast_matchers::MatchFinder::MatchResult ,
+ const CastExprT *CastExpr);
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -31,6 +31,11 @@
   unless(isInTemplateInstantiation()))
   .bind("cast"),
   this);
+  Finder->addMatcher(
+  cxxFunctionalCastExpr(unless(hasDescendant(cxxConstructExpr())),
+unless(hasDescendant(initListExpr(
+  .bind("functional_cast"),
+  this);
 }
 
 static bool needsConstCast(QualType SourceType, QualType DestType) {
@@ -57,7 +62,18 @@
 
 void AvoidCStyleCastsCheck::check(const MatchFinder::MatchResult ) {
   const auto *CastExpr = Result.Nodes.getNodeAs("cast");
+  const auto *FunctionalCastExpr =
+  Result.Nodes.getNodeAs("functional_cast");
+
+  if (CastExpr)
+checkExpr(Result, CastExpr);
+  else if (FunctionalCastExpr)
+checkExpr(Result, FunctionalCastExpr);
+}
 
+template 
+void AvoidCStyleCastsCheck::checkExpr(const MatchFinder::MatchResult ,
+  const CastExprT *CastExpr) {
   // Ignore casts in macros.
   if (CastExpr->getExprLoc().isMacroID())
 return;
@@ -80,8 +96,14 @@
   const QualType SourceType =