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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

On Mon, Oct 5, 2015 at 6:14 PM, Matthias Gehre <M.Gehre at gmx.de> wrote:

> mgehre marked an inline comment as done.

> 

> ================

>  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.");

>  +  }

> 

>  ----------------

> 

> aaron.ballman wrote:

> 

> > What is the alternative the user should use in this instance?

> 

> 

> The alternative is to either

> 

> 1. change the program logic to not require and downcast of a non-polymorphic type or

> 2. add NOLINT to notify your coworkers that this may be the spot where something goes wrong

> 

>   In the end, these rules together should eliminate some classes of undefined behavior and crashes. If the application still crashes, you should just need to look at the (hopefully few) places of NOLINT.

> 

>   Anyway, I didn't make the rules. See Herb Sutters answer here: https://github.com/isocpp/CppCoreGuidelines/issues/270


This wasn't a comment on the rule so much as a comment on the diagnostic not being very helpful.In this case, you're telling the user to not do something, but it is unclear how the user would structure their code to silence this diagnostic. Perhaps there is no way to word this to give the user a clue, but we should at least try. If I got this diagnostic as it is now, I would scratch my head and quickly decide to ignore it.

> ================

>  Comment at: clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp:33

>  @@ +32,3 @@

>  +  const auto *SourceDecl = SourceType->getPointeeCXXRecordDecl();

>  +  if(!SourceDecl)

>  +    SourceDecl = SourceType->getAsCXXRecordDecl();

> 

>  ----------------

> 

> aaron.ballman wrote:

> 

> > In the event it's not a pointer or a reference, why are you getting the source as a value type?

> 

> 

> Source type could be no pointer nor ref like in:

> 

>   Base B0;

>   auto R0 = static_cast<Derived&>(B0);


Ah, interesting! I was guessing that case would have had an lvalue reference type for B0, but I'm guessing it's already gone through lvalue to rvalue conversion before hitting the cast node (or my intuition is just off).


================
Comment at: test/clang-tidy/cppcoreguidelines-pro-type-static-cast-downcast.cpp:39
@@ +38,3 @@
+  auto PP0 = static_cast<PolymorphicDerived*>(new PolymorphicBase());
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use static_cast to downcast from a base to a derived class; use dynamic_cast instead [cppcoreguidelines-pro-type-static-cast-downcast]
+  // CHECK-FIXES: auto PP0 = dynamic_cast<PolymorphicDerived*>(new PolymorphicBase());
----------------
No need to have the [cppcoreguidelines-pro-type-static-cast-downcast] part of the message on anything but the first diagnostic in the file. (This reduces clutter, but we test it one time just to make sure it's there).


http://reviews.llvm.org/D13368