================ Comment at: clang-tidy/google/CMakeLists.txt:4 @@ -3,2 +3,3 @@ add_clang_library(clangTidyGoogleModule + CStyleCastsCheck.cpp ExplicitConstructorCheck.cpp ---------------- Daniel Jasper wrote: > 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. 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. ================ Comment at: clang-tidy/google/CStyleCastsCheck.h:21 @@ +20,3 @@ +/// +/// 'readability/casting' +/// http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Casting#Casting ---------------- Daniel Jasper wrote: > What is this? This is the name of the corresponding check in cpplint.py. I've added a comment. ================ Comment at: clang-tidy/google/GoogleTidyModule.cpp:25 @@ -23,1 +24,3 @@ CheckFactories.addCheckFactory( + "google-readability-casting", + new ClangTidyCheckFactory<readability::CStyleCastsCheck>()); ---------------- Daniel Jasper wrote: > Maybe order alphabetically by check name? Done. ================ Comment at: test/clang-tidy/c-style-casts.cpp:1 @@ +1,2 @@ +// RUN: clang-tidy -checks=-*,google-readability-casting %s -- | FileCheck %s + ---------------- Daniel Jasper wrote: > Maybe use check_clang_tidy_output.sh to decouple the check configuration. 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. ================ Comment at: test/clang-tidy/c-style-casts.cpp:3 @@ +2,3 @@ + +// CHECK-NOT: warning: +bool g() { return false; } ---------------- Daniel Jasper wrote: > Out of curiosity, why would there be a c-style cast here? 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. http://reviews.llvm.org/D4189 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
