Page MenuHomePhabricator

[clang-tidy] add new check cppcoreguidelines-pro-type-cstyle-cast
ClosedPublic

Authored by mgehre on Oct 26 2015, 4:04 PM.

Details

Summary

This check flags all use of c-style casts that perform a static_cast
downcast, const_cast, or reinterpret_cast.

Use of these casts can violate type safety and cause the program to
access a
variable that is actually of type X to be accessed as if it were of an
unrelated type Z. Note that a C-style (T)expression cast means to
perform
the first of the following that is possible: a const_cast, a
static_cast, a
static_cast followed by a const_cast, a reinterpret_cast, or a
reinterpret_cast followed by a const_cast. This rule bans (T)expression
only when used to perform an unsafe cast.

This rule is part of the "Type safety" profile of the C++ Core
Guidelines, see
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-type4-dont-use-c-style-texpression-casts-that-would-perform-a-static_cast-downcast-const_cast-or-reinterpret_cast.

Diff Detail

Repository
rL LLVM

Event Timeline

mgehre updated this revision to Diff 38477.Oct 26 2015, 4:04 PM
mgehre retitled this revision from to [clang-tidy] add new check cppcoreguidelines-pro-type-cstyle-cast.
mgehre updated this object.
mgehre added a subscriber: cfe-commits.
sbenza added inline comments.Oct 26 2015, 4:25 PM
clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp
44 ↗(On Diff #38477)

Seems to be missing CK_ReinterpretMemberPointer

48 ↗(On Diff #38477)

From reading warning this I would think the way to fix the code is to use a reinterpret_cast directly, but that is also against the guidelines.

55 ↗(On Diff #38477)

remove blank line

There is already a similar check in the Google package. What are the differences between those two checks? What is the reason we can not just register that check into the core guidelines module?

sbenza edited edge metadata.Oct 27 2015, 11:10 AM

There is already a similar check in the Google package. What are the differences between those two checks? What is the reason we can not just register that check into the core guidelines module?

That other check discourages c-style cast in favor of C++ style casts, even if it is a reinterpret_cast. It simply replaces the cstyle cast with an equivalent C++ one. It is basically a stylistic check.

This check will warn unsafe cstyle casts, while allowing safe ones like int->uint casts.
This one is a safety related check.

mgehre updated this revision to Diff 38857.Nov 1 2015, 2:00 PM
mgehre edited edge metadata.

Review comments: Formating and changed diag messages

aaron.ballman added inline comments.Nov 1 2015, 6:04 PM
clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp
49 ↗(On Diff #38857)

"C-style" instead of "c-style"?

57 ↗(On Diff #38857)

Missing period to terminate the comment.

74 ↗(On Diff #38857)

"C-style"

79 ↗(On Diff #38857)

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

80 ↗(On Diff #38857)

Why do we need this check? Perhaps we want to use IgnoreParenImpCasts() instead?

94 ↗(On Diff #38857)

C-style

101 ↗(On Diff #38857)

C-style

docs/clang-tidy/checks/cppcoreguidelines-pro-type-cstyle-cast.rst
4 ↗(On Diff #38857)

C-style

mgehre added inline comments.Nov 2 2015, 11:57 AM
clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp
79 ↗(On Diff #38857)

I copied the fixit part from the google c-style cast check. I will add some more test for it.

mgehre added inline comments.Nov 2 2015, 1:43 PM
clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp
80 ↗(On Diff #38857)

If the cast looks like

Derived* D = (Derived*)B;

we need to add parenthesis around B to turn it into

Derived* D = dynamic_cast<Derived*>(B);

If instead we already had

Derived* D = (Derived*)(B);

adding the parenthesis can be skipped.

mgehre updated this revision to Diff 38981.Nov 2 2015, 1:43 PM

Update for review comments

alexfh added a subscriber: alexfh.Nov 5 2015, 2:20 PM

A peanut gallery comment.

clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp
91 ↗(On Diff #38981)

Please add braces around the else body.

mgehre updated this revision to Diff 39576.Nov 6 2015, 12:35 PM

Update for review comments: add braces around else

alexfh accepted this revision.Nov 6 2015, 6:55 PM
alexfh added a reviewer: alexfh.

Looks good with a comment. Thank you for contribution!

test/clang-tidy/cppcoreguidelines-pro-type-cstyle-cast.cpp
14 ↗(On Diff #39576)

I think, it makes sense to leave the [cppcoreguidelines-pro-type-cstyle-cast] only once and remove it everywhere else to make the test easier to read.

This revision is now accepted and ready to land.Nov 6 2015, 6:55 PM
This revision was automatically updated to reflect the committed changes.