[PATCH] D14096: [clang-tidy] add new check cppcoreguidelines-pro-type-cstyle-cast

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp:49
@@ +48,3 @@
+    diag(MatchedCast->getLocStart(),
+         "do not use c-style cast to convert between unrelated types");
+    return;
----------------
"C-style" instead of "c-style"?

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp:57
@@ +56,3 @@
+    const auto *SourceDecl = SourceType->getPointeeCXXRecordDecl();
+    if (!SourceDecl) // The cast is from object to reference
+      SourceDecl = SourceType->getAsCXXRecordDecl();
----------------
Missing period to terminate the comment.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp:74
@@ +73,3 @@
+          MatchedCast->getLocStart(),
+          "do not use c-style cast to downcast from a base to a derived class; "
+          "use dynamic_cast instead");
----------------
"C-style"

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp:79
@@ +78,3 @@
+          MatchedCast->getSubExprAsWritten()->IgnoreImpCasts();
+      std::string CastText = ("dynamic_cast<" + DestTypeString + ">").str();
+      if (!isa<ParenExpr>(SubExpr)) {
----------------
How does this handle a case like:
```
SomeClass::type *derived = (SomeClass::type *)someBase;
```
Will it leave the spelling as "SomeClass::type *", or replace it with the underlying type from the typedef? (I suspect it leaves it as-is, but it would be good to have tests for more complex code patterns.)

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp:80
@@ +79,3 @@
+      std::string CastText = ("dynamic_cast<" + DestTypeString + ">").str();
+      if (!isa<ParenExpr>(SubExpr)) {
+        CastText.push_back('(');
----------------
Why do we need this check? Perhaps we want to use IgnoreParenImpCasts() instead?

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp:94
@@ +93,3 @@
+          MatchedCast->getLocStart(),
+          "do not use c-style cast to downcast from a base to a derived class");
+    return;
----------------
C-style

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp:101
@@ +100,3 @@
+    diag(MatchedCast->getLocStart(),
+         "do not use c-style cast to cast away constness");
+  }
----------------
C-style

================
Comment at: docs/clang-tidy/checks/cppcoreguidelines-pro-type-cstyle-cast.rst:4
@@ +3,3 @@
+
+This check flags all use of c-style casts that perform a static_cast downcast, const_cast, or reinterpret_cast.
+
----------------
C-style


http://reviews.llvm.org/D14096