Page MenuHomePhabricator

Add a check to flag the usage of C-style casts (Google Style).
ClosedPublic

Authored by alexfh on Jun 18 2014, 2:56 AM.

Diff Detail

Event Timeline

alexfh updated this revision to Diff 10541.Jun 18 2014, 2:56 AM
alexfh retitled this revision from to Add a check to flag the usage of C-style casts (Google Style)..
alexfh updated this object.
alexfh edited the test plan for this revision. (Show Details)
alexfh added a reviewer: djasper.
alexfh added a subscriber: Unknown Object (MLST).
djasper added inline comments.Jun 18 2014, 3:10 AM
clang-tidy/google/CMakeLists.txt
4

I think the naming is not ideal. First, the "..Check" is mostly redundant information, maybe remove that, at least for new checks. Second, it would be good to verify what is being checked with c-style casts. So, maybe DontUseCStyleCasts.cpp or AvoidCStyleCasts.cpp.

clang-tidy/google/CStyleCastsCheck.h
22

What is this?

clang-tidy/google/GoogleTidyModule.cpp
25

Maybe order alphabetically by check name?

test/clang-tidy/c-style-casts.cpp
2

Maybe use check_clang_tidy_output.sh to decouple the check configuration.

4

Out of curiosity, why would there be a c-style cast here?

alexfh added inline comments.Jun 18 2014, 5:24 AM
clang-tidy/google/CMakeLists.txt
4

I see some value in using the "Check" suffix for checks: it makes class names nouns in a consistent way. I like it a bit more than using imperatives (e.g. UseOverride). The matter of the check can rarely be expressed using a single short imperative statement.

A few examples:

  • ArgumentCommentCheck checks that the correct argument name is used in the argument comment. I don't see a good alternative name in the imperative style (UseCorrectArgumentNameInArgumentComment?).
  • one could name a check NameLocalVariablesCorrectly or LocalVariableNamingCheck. The latter seems a better alternative to me (though, it's a matter of taste).
  • UseOverride check doesn't always ask you to "use override". It checks the correct usage of the virtual, override and final keywords on methods. In this case the name reflects the most frequent use case of the check (and is rather clear), but the name hints that this is the _only_ use case. An alternative name using noun notation could be more correct (but maybe a bit more bulky): VirtualOverrideFinalCheck or InheritanceSpecifierCheck.

That said, AvoidCStyleCasts (though, I would prefer AvoidCStyleCastsCheck) may be an appropriate name for the check in this patch.

clang-tidy/google/CStyleCastsCheck.h
22

This is the name of the corresponding check in cpplint.py. I've added a comment.

clang-tidy/google/GoogleTidyModule.cpp
25

Done.

test/clang-tidy/c-style-casts.cpp
2

This script brings a bit of convenience, but it adds a requirement to use shell for tests, which limits the environments where the tests can run. However, the other script - check_clang_tidy_fix.sh - brings significant value, as it reduces a lot of shell code duplication from the RUN lines in the tests.

So I'd avoid using check_clang_tidy_output.sh. I'm not sure we actually need it, maybe just for consistency in cases where we have tests both for output and for fixes.

4

There's no c-style cast here. It's a utility function used below. The placement of the CHECK-NOT may be confusing though. I'll put a blank line here, so that it doesn't look like it relates to this function. We need the CHECK-NOT before the first CHECK as a generic measure to make the check more strict.

alexfh updated this revision to Diff 10556.Jun 18 2014, 5:25 AM

Reordered registration of the checks. Added a newline after CHECK-NOT.

djasper accepted this revision.Jun 25 2014, 1:43 AM
djasper edited edge metadata.

Basically looks good, none of these other things are blockers.

clang-tidy/google/CMakeLists.txt
4

I think there are pros/cons for many of these naming schemes. I'd mostly like us to settle on one and then be as consistent as possible.

Regarding your arguments:

  • LocalVariableNamingCheck pretty precisely describes what it does, so that name is fine.
  • ArgumentCommentCheck could intuitively also mean that it checks that arguments have comments. As we might eventually extend the check to require/add arguments for literals used as comments, this might be fine. Alternatively, we could rename it to something like ArgumentCommentNamingCheck.
  • I agree that UseOverride might not be the best name. Generally, choosing the name based on the most frequent case does not seem like a bad idea, though. Maybe we should call it PreferOverrideCheck? This adds "..Check" and slightly weakens the hint that it is the only use case.
  • I'd still prefer AvoidCStyleCastsCheck over CStyleCastsCheck, as CStyleCastsCheck does not at all say, what it does. It could also check various other things (spacing, redundant casts, ..).
test/clang-tidy/c-style-casts.cpp
2

Well, I think we either need it or not. Consistency for cases where we have a fix seems like a bad distinction as we will hopefully have fixes for almost all checks (including this one).

We can fix this later (don't want to block this patch on it), but to me it seems like we should write the tests consistently (and fix this soon, before we have hundreds of tests). I don't care which way, my data-point would be that parameter changes to clang-tidy (like we have done with how the checks are configured) are significantly easier when clang-tidy is wrapped.

This revision is now accepted and ready to land.Jun 25 2014, 1:43 AM
alexfh closed this revision.Jun 25 2014, 8:00 AM