[PATCH] D13368: [clang-tidy] add check cppcoreguidelines-pro-type-static-cast-downcast

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp:33
@@ +32,3 @@
+  const auto *SourceDecl = SourceType->getPointeeCXXRecordDecl();
+  if(!SourceDecl)
+    SourceDecl = SourceType->getAsCXXRecordDecl();
----------------
In the event it's not a pointer or a reference, why are you getting the source as a value type?

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp:46
@@ +45,3 @@
+
+  if (SourceDecl->isPolymorphic()) {
+    diag(MatchedCast->getOperatorLoc(), "do not use static_cast to cast from base class to derived class. "
----------------
You should elide the braces here and for the else statement.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp:47
@@ +46,3 @@
+  if (SourceDecl->isPolymorphic()) {
+    diag(MatchedCast->getOperatorLoc(), "do not use static_cast to cast from base class to derived class. "
+      "Use dynamic_cast instead.")
----------------
Punctuation nit -- we don't use complete sentences for diagnostics. This should be something like:

do not use static_cast to downcast from a base to a derived class; use dynamic_cast instead

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp:50
@@ +49,3 @@
+      << FixItHint::CreateReplacement(
+          {MatchedCast->getOperatorLoc(), MatchedCast->getOperatorLoc()},
+          "dynamic_cast");
----------------
I am pretty sure that you can create the replacement from a SourceLocation directly, because SourceRange has a nonexplicit constructor accepting a single SourceLocation.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp:53
@@ +52,3 @@
+  } else {
+    diag(MatchedCast->getOperatorLoc(), "do not use static_cast to cast from base class to derived class.");
+  }
----------------
What is the alternative the user should use in this instance?


http://reviews.llvm.org/D13368