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

[Aaron Ballman via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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

> mgehre marked an inline comment as done.

>  mgehre added a comment.

> 

> I cannot think of any way to improve the usefulness of the diagnostic in the non-polymorphic case.

>  For now, this patch has a link to the CppCoreGuidelines document in the docs/**/*.rst file.


Neither can I, so I guess it stands for right now.

> Also, this check is not enabled by default. One has to explicitly enable it, and may at that

>  point reason about the usefulness of the CppCoreGuidelines and this check.


That is true, but enabling this check doesn't always require thoughtfulness on the part of the user. They can enable the check with -checks=corecppguidelines-* for instance. However, they can at least find the documentation from the check failures and hopefully go from there.

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

>  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());

> 

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

> 

> aaron.ballman wrote:

> 

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

> 

> 

> I can remove it on the remaining messages that end in "use dynamic_cast instead". I need to keep it in the non-polymorphic case

>  to make sure that they are not followed by "use dynamic_cast instead".


Ah, that's an excellent point.

Patch LGTM! Since Samuel had comments as well, I will wait for his sign-off before committing.

~Aaron


http://reviews.llvm.org/D13368