Index: clang-tidy/cppcoreguidelines/CMakeLists.txt =================================================================== --- clang-tidy/cppcoreguidelines/CMakeLists.txt +++ clang-tidy/cppcoreguidelines/CMakeLists.txt @@ -2,6 +2,7 @@ add_clang_library(clangTidyCppCoreGuidelinesModule AvoidGotoCheck.cpp + ConstCheck.cpp CppCoreGuidelinesTidyModule.cpp InterfacesGlobalInitCheck.cpp NoMallocCheck.cpp Index: clang-tidy/cppcoreguidelines/ConstCheck.h =================================================================== --- /dev/null +++ clang-tidy/cppcoreguidelines/ConstCheck.h @@ -0,0 +1,68 @@ +//===--- ConstCheck.h - clang-tidy-------------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_CONSTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_CONSTCHECK_H + +#include "../ClangTidy.h" +#include +#include + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +/// This check warns for every variable, that could be declared as const, but +/// isn't. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-const.html +class ConstCheck : public ClangTidyCheck { +public: + ConstCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void onEndOfTranslationUnit() override; + +private: + void variableRegistering(ast_matchers::MatchFinder *Finder); + void handleRegistration(const ast_matchers::MatchFinder::MatchResult &Result); + + void valueTypeMatchers(ast_matchers::MatchFinder *Finder); + void checkValueType(const ast_matchers::MatchFinder::MatchResult &Result); + void invalidateRefCaptured(const LambdaExpr *Lambda); + + template + bool notConstVal(const ast_matchers::MatchFinder::MatchResult &Result, + StringRef matcher_bind) { + if (Result.Nodes.getNodeAs(matcher_bind)) { + //std::cout << "Prohibting through " << std::string(matcher_bind) + //<< std::endl; + const auto *Variable = Result.Nodes.getNodeAs("value-decl"); + ValueCanBeConst[Variable] = false; + return true; + } + return false; + } + + void handleTypeMatchers(ast_matchers::MatchFinder *Finder); + void checkHandleType(const ast_matchers::MatchFinder::MatchResult &Result); + + void diagnosePotentialConst(); + + std::unordered_map ValueCanBeConst; + std::unordered_map HandleCanBeConst; +}; + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_CONSTCHECK_H Index: clang-tidy/cppcoreguidelines/ConstCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/cppcoreguidelines/ConstCheck.cpp @@ -0,0 +1,395 @@ +//===--- ConstCheck.cpp - clang-tidy---------------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "ConstCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +/* + * General Thoughts + * ================ + * + * For now: Only local variables are considered. Globals/namespace variables, + * paramters and class members are not analyzed. + * Parameters have a check already: readability-non-const-parameter + * + * + * Handle = either a pointer or reference + * Value = everything else (Type variable_name;) + * + * Value Semantic + * ============== + * - it is neither global nor namespace level + CHECK + * - it never gets assigned to after initialization + CHECK + * -> every uninitialized variable can not be const + CHECK + * - no non-const handle is created with it + CHECK + * - no non-const pointer from it + CHECK + * - no non-const pointer argument + CHECK + * - no non-const reference from it + CHECK + * - no non-const reference argument + CHECK + * - no non-const capture by reference in a lambda + CHECK + * - it is not returned as non-const handle from a function + CHECK + * - it address is not assigned to an out pointer parameter + CHECK + * + * primitive Builtins + * ---------------- + * - it is not modified with an operator (++i,i++,--i,i--) + CHECK + * - it is not modified with an operator-assignment + CHECK + * + * objects + * ------- + * - there is no call to a non-const method + * - there is no call to an non-const overloaded operator + * - there is no non-const iterator created from this type + * (std::begin and friends) + * + * arrays + * ------ + * - there is no non-const operator[] access + * - there is no non-const handle creation of one of the elements + * - there is no non-const iterator created from this type + * (std::begin and friends) + * + * templated variables + * ------------------- + * - more dificult to reason about + * - everything from the untemplated sections apply here + * + * - Used as argument to a function having a non-const overload + * - Method call that has a non-const overload + * - any call to operators, as they can be overloaded + * + * -> Everything can be overloaded, only a call to a function, that + * requires a const value as argument is a safe const. That must have + * been declared const already + * -> Values in Templates can not be reasoned about. + * + * Ignore it for now? + * + * Handle Semantic + * =============== + * - modification of the pointee prohibit constness + * + * pointers + * -------- + * - match both for value and handle semantic + * + * references + * ---------- + * - only handle semantic applies + * - references to templated types? + * + * forwarding reference + * -------------------- + * - same as references? + * + * Implementation strategy + * ======================= + * + * - Register every declared local variable/constant with value semantic. + * (pointers, values) + * Store if they can be made const. + * (const int i = 10 : no, + * int *const = &i : no, + * int i = 10 : yes, -> const int i = 10 + * int *p_i = &i : yes, -> int *const p_i = &i) + * - Register every declared local variable/constant with handle semantic. + * (pointers, references) + * Store if they can be made const, meaning if they can be a const target + * (const int *cp_i = &i : no, + * const int &cr_i = i : no, + * int *p_i = &i : yes, -> const int *p_i = &i + * int &r_i = i : yes, -> const int &r_i = i) + * - Keep 2 dictionaries for values and handles + * + * - Match operations/events that forbid values to be const -> mark then 'no' + * - Match operations/events that forbid handles to be const -> mark then 'no' + * + * - once the translation unit is finished, determine what can be const, by + * just iterating over all keys and check if they map to 'true'. + * - values that can be const -> emit warning for their type and name + * - handles that can be const -> emit warning for the pointee type and name + * - ignore the rest + * + * Caveats + * ======= + * + * - values of templated types are tricky, because an other + * instantiation might not have the same const-properties + * -> remove const if one of this happens: + * - operator call (overloaded operators) + * - method call + * - overloaded function call (normal functions) + * + * Open Questions + * ============== + * + * - what is if a type does not provide a const overload for some operation + * -> is every usage of this operation non-const? (operator[]) (Yes?!) + * - how to deal with range-for? -> the iterator var is just another variable + * - is iterator creation already covered by the usual method-call and function + * calls (should be!, but implicit calls in range-based for?) + * - type conversions: + * - one can overload the type conversion operation and modify a value of a + * class -> implications? + * - what about the 'mutable' keyword + * - handles to templated types + */ + +void ConstCheck::registerMatchers(MatchFinder *Finder) { + // Ensure that all intersting variables are registered in our mapping. + variableRegistering(Finder); + + valueTypeMatchers(Finder); + // ----------------- matchers for pointer semantic --------------------------- +} + +void ConstCheck::check(const MatchFinder::MatchResult &Result) { + if (!getLangOpts().CPlusPlus) + return; + + handleRegistration(Result); + checkValueType(Result); + // checkHandleType(Result); +} + +// The decision which variable might be made const can only be made at the +// end of each translation unit. +void ConstCheck::onEndOfTranslationUnit() { diagnosePotentialConst(); } + +void ConstCheck::variableRegistering(MatchFinder *Finder) { + const auto IsHandleType = + anyOf(hasType(referenceType()), hasType(pointerType())); + const auto IsValueType = unless(hasType(referenceType())); + + // Match value and pointer access. + // Pointer do have value and reference semantic. + const auto IsValueDeclRefExpr = declRefExpr( + allOf(hasDeclaration(varDecl().bind("var-decl")), IsValueType)); + // Match pointer and reference access. + const auto IsHandleDeclRefExpr = declRefExpr( + allOf(hasDeclaration(varDecl().bind("handle-decl")), IsHandleType)); + + const auto ConstType = hasType(isConstQualified()); + const auto TemplateType = hasType(templateTypeParmType()); + const auto LocalVariable = + anyOf(hasAncestor(functionDecl(hasBody(compoundStmt()))), + hasAncestor(lambdaExpr())); + + // Match global and namespace wide variables that will never be diagnosed. + // Global handles are not relevant, because this check does not analyze that + // for now. + /* Why is this necessary again? + Finder->addMatcher(varDecl(allOf(noneOf(hasAncestor(functionDecl()), + hasAncestor(lambdaExpr())), + hasInitializer(anything()), IsValueType)) + .bind("new-global-value"), + this); + */ + + // Match local variables, that could be const. + // Example: `int i = 10`, `int i` (will be used if program is correct) + Finder->addMatcher( + varDecl(allOf(LocalVariable, hasInitializer(anything()), + unless(ConstType), unless(TemplateType), IsValueType)) + .bind("new-local-value"), + this); + // Match local constants. + // Example: `const int ri = 1` + /* Not necessary?! + Finder->addMatcher(varDecl(allOf(LocalVariable, ConstType, IsValueType)) + .bind("new-local-const-value"), + this); + */ + + // Match local handle types, that are not const. + // Example: `int &ri`, `int * pi`. + Finder->addMatcher( + varDecl(allOf(LocalVariable, IsHandleType, unless(ConstType))) + .bind("new-local-handle"), + this); +} + +void ConstCheck::handleRegistration(const MatchFinder::MatchResult &Result) { + // Local variables can be declared as consts. + if (const auto *Variable = + Result.Nodes.getNodeAs("new-local-value")) { + if (ValueCanBeConst.find(Variable) == ValueCanBeConst.end()) { + ValueCanBeConst[Variable] = true; + return; + } + } + + if (const auto *Variable = + Result.Nodes.getNodeAs("new-local-handle")) { + if (HandleCanBeConst.find(Variable) == HandleCanBeConst.end()) { + HandleCanBeConst[Variable] = true; + return; + } + } +} + +void ConstCheck::valueTypeMatchers(MatchFinder *Finder) { + const auto IsHandleType = + anyOf(hasType(referenceType()), hasType(pointerType())); + const auto IsValueType = unless(hasType(referenceType())); + + // Match value and pointer access. + // Pointer do have value and reference semantic. + const auto IsValueDeclRefExpr = declRefExpr( + allOf(hasDeclaration(varDecl().bind("value-decl")), IsValueType)); + + // Classical assignment of any form (=, +=, <<=, ...) modifies the LHS + // and prohibts it from being const. + Finder->addMatcher( + binaryOperator(allOf(isAssignmentOperator(), hasLHS(IsValueDeclRefExpr))) + .bind("value-assignment"), + this); + + // Usage of the '++' or '--' operator modifies a variable. + Finder->addMatcher( + unaryOperator(allOf(anyOf(hasOperatorName("++"), hasOperatorName("--")), + hasUnaryOperand(IsValueDeclRefExpr))) + .bind("value-unary-modification"), + this); + + // Check the address operator. + Finder->addMatcher( + unaryOperator(allOf(hasOperatorName("&"), + // Checking for the ImplicitCastExpr is enough, + // because a pointer can get casted only in the 'add + // const' direction implicitly. + unless(hasParent(implicitCastExpr())), + hasUnaryOperand(IsValueDeclRefExpr))) + .bind("value-address-to-non-const"), + this); + + const auto RefenceNonConstValueType = hasType(references( + qualType(allOf(unless(referenceType()), unless(isConstQualified()))))); + // Check creation of references to this value. + Finder->addMatcher(varDecl(allOf(RefenceNonConstValueType, + hasInitializer(IsValueDeclRefExpr))) + .bind("value-non-const-reference"), + this); + + // Check function calls that bind by reference. + Finder->addMatcher( + callExpr(forEachArgumentWithParam( + IsValueDeclRefExpr, parmVarDecl(RefenceNonConstValueType) + .bind("value-non-const-ref-call-param"))), + this); + + // Check return values that reference a value. + Finder->addMatcher( + functionDecl( + allOf(hasDescendant(returnStmt(hasReturnValue(IsValueDeclRefExpr))), + returns(qualType( + references(qualType(unless(isConstQualified()))))))) + .bind("returns-non-const-ref"), + this); + + // Lambda expression can capture variables by reference which invalidates + // the captured variables. Lambdas capture only the variables they actually + // use! + Finder->addMatcher(lambdaExpr().bind("value-lambda"), this); +} + +void ConstCheck::checkValueType(const MatchFinder::MatchResult &Res) { + bool Prohibited = false; + + // Assignment of any form prohibits the LHS to be const. + Prohibited = notConstVal(Res, "value-assignment"); + // Usage of the '++' or '--' operator modifies a value. + Prohibited |= notConstVal(Res, "value-unary-modification"); + // The address of the values has been taken and did not result in a + // pointer to const. + Prohibited |= notConstVal(Res, "value-address-to-non-const"); + // A reference is initialized with a value. + Prohibited |= notConstVal(Res, "value-non-const-reference"); + // A call where a parameter is a non-const reference. + Prohibited |= notConstVal(Res, "value-non-const-ref-call-param"); + // A function returning a non-const reference prohibts its return value + // to be const. + Prohibited |= notConstVal(Res, "returns-non-const-ref"); + + // If the matchable targets did result in prohibiton, we can stop. + // Otherwise analysis that does not function with the same pattern must be + // applied. + if (Prohibited) + return; + + // Analysis of the lambda is more difficult. + // Offloaded into a separate function. + if (const auto *Lambda = Res.Nodes.getNodeAs("value-lambda")) { + invalidateRefCaptured(Lambda); + return; + } +} + +void ConstCheck::invalidateRefCaptured(const LambdaExpr *Lambda) { + for (const auto capture : Lambda->captures()) { + if (capture.capturesVariable() && + capture.getCaptureKind() == LambdaCaptureKind::LCK_ByRef) { + ValueCanBeConst[capture.getCapturedVar()] = false; + } + } +} + +void ConstCheck::diagnosePotentialConst() { + // Diagnose all potential values. + for (const auto it : ValueCanBeConst) { + bool VarCanBeConst = it.second; + + if (VarCanBeConst) { + const VarDecl *Variable = it.first; + + diag(Variable->getLocStart(), + "variable %0 of type %1 can be declared const") + << Variable << Variable->getType(); + } + } + + /* + for (const auto it : HandleCanBeConst) { + bool HandleCanBeConst = it.second; + + // Example: `int& ri` could be `const int& ri`. + if (HandleCanBeConst) { + const VarDecl *Variable = it.first; + + // Differentiate between pointers and references. + QualType HandleType = Variable->getType(); + if (HandleType->isReferenceType()) { + + diag(Variable->getLocStart(), + "reference type %0 of variable %1 can be declared const") + << HandleType << Variable; + } else if (HandleType->isPointerType()) { + + diag(Variable->getLocStart(), + "pointee type %0 of variable %1 can be declared const") + << HandleType << Variable; + } else + llvm_unreachable("Expected handle type"); + } + } + */ +} + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang Index: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp =================================================================== --- clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp @@ -12,6 +12,7 @@ #include "../ClangTidyModuleRegistry.h" #include "../misc/UnconventionalAssignOperatorCheck.h" #include "AvoidGotoCheck.h" +#include "ConstCheck.h" #include "InterfacesGlobalInitCheck.h" #include "NoMallocCheck.h" #include "OwningMemoryCheck.h" @@ -38,6 +39,8 @@ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck( "cppcoreguidelines-avoid-goto"); + CheckFactories.registerCheck( + "cppcoreguidelines-const"); CheckFactories.registerCheck( "cppcoreguidelines-interfaces-global-init"); CheckFactories.registerCheck("cppcoreguidelines-no-malloc"); Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -57,6 +57,11 @@ Improvements to clang-tidy -------------------------- +- New :doc:`cppcoreguidelines-const + ` check + + FIXME: add release notes. + - New module `abseil` for checks related to the `Abseil `_ library. Index: docs/clang-tidy/checks/cppcoreguidelines-const.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/cppcoreguidelines-const.rst @@ -0,0 +1,6 @@ +.. title:: clang-tidy - cppcoreguidelines-const + +cppcoreguidelines-const +======================= + +FIXME: Describe what patterns does the check detect and why. Give examples. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -75,6 +75,7 @@ cert-oop11-cpp (redirects to performance-move-constructor-init) cppcoreguidelines-avoid-goto cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) + cppcoreguidelines-const cppcoreguidelines-interfaces-global-init cppcoreguidelines-no-malloc cppcoreguidelines-owning-memory Index: test/clang-tidy/cppcoreguidelines-const-primtive-builtin.cpp =================================================================== --- /dev/null +++ test/clang-tidy/cppcoreguidelines-const-primtive-builtin.cpp @@ -0,0 +1,187 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-const %t + +// ------- Provide test samples for primitive builtins --------- +// - every 'p_*' variable is a 'potential_const_*' variable +// - every 'np_*' variable is a 'non_potential_const_*' variable + +bool global; +char np_global = 0; // globals can't be known to be const + +namespace foo { +int scoped; +float np_scoped = 1; // namespace variables are like globals +} // namespace foo + +void some_function(double, wchar_t); + +void some_function(double np_arg0, wchar_t np_arg1) { + int p_local0 = 2; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const + + int np_local0; + const int np_local1 = 42; + + unsigned int np_local2 = 3; + np_local2 <<= 4; + + int np_local3 = 4; + ++np_local3; + int np_local4 = 4; + np_local4++; + + int np_local5 = 4; + --np_local5; + int np_local6 = 4; + np_local6--; +} + +void some_lambda_environment_capture_all_by_reference(double np_arg0) { + int np_local0 = 0; + int p_local0 = 1; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const + + int np_local2; + const int np_local3 = 2; + + // Capturing all variables by reference prohibits making them const. + [&]() { ++np_local0; }; + + int p_local1 = 0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const +} + +void some_lambda_environment_capture_all_by_value(double np_arg0) { + int np_local0 = 0; + int p_local0 = 1; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const + + int np_local1; + const int np_local2 = 2; + + // Capturing by value has no influence on them. + [=]() { (void)p_local0; }; + + np_local0 += 10; +} + +void function_inout_pointer(int *inout); +void function_in_pointer(const int *in); + +void some_pointer_taking(int *out) { + int np_local0 = 42; + const int *const p0_np_local0 = &np_local0; + int *const p1_np_local0 = &np_local0; + + int np_local1 = 42; + function_inout_pointer(&np_local1); + + // Prevents const. + int np_local2 = 42; + out = &np_local2; // This returns and invalid address, its just about the AST + + int p_local0 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const + const int *const p0_p_local0 = &p_local0; + + int p_local1 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const + function_in_pointer(&p_local1); +} + +void function_inout_ref(int &inout); +void function_in_ref(const int &in); + +void some_reference_taking() { + int np_local0 = 42; + const int &r0_np_local0 = np_local0; + int &r1_np_local0 = np_local0; + const int &r2_np_local0 = r1_np_local0; + + int np_local1 = 42; + function_inout_ref(np_local1); + + int p_local0 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const + const int &r0_p_local0 = p_local0; + + int p_local1 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const + function_in_ref(p_local1); +} + +double *non_const_pointer_return() { + double p_local0 = 0.0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const + double np_local0 = 24.4; + + return &np_local0; +} + +const double *const_pointer_return() { + double p_local0 = 0.0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const + double p_local1 = 24.4; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared const + return &p_local1; +} + +double &non_const_ref_return() { + double p_local0 = 0.0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const + double np_local0 = 42.42; + return np_local0; +} + +const double &const_ref_return() { + double p_local0 = 0.0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const + double p_local1 = 24.4; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared const + return p_local1; +} + +double *&return_non_const_pointer_ref() { + double * np_local0 = nullptr; + return np_local0; +} + +void overloaded_arguments(const int &in); +void overloaded_arguments(int &inout); +void overloaded_arguments(const int *in); +void overloaded_arguments(int *inout); + +void function_calling() { + int np_local0 = 42; + overloaded_arguments(np_local0); + + const int np_local1 = 42; + overloaded_arguments(np_local1); + + int np_local2 = 42; + overloaded_arguments(&np_local2); + + const int np_local3 = 42; + overloaded_arguments(&np_local3); +} + +// Now comes the tricky part. Templates + +// deactivate delayed parsing +template +void define_locals(T np_arg0, T &np_arg1, int np_arg2) { + T np_local0 = 0; + np_local0 += np_arg0 * np_arg1; + + T np_local1 = 42; + np_local0 += np_local1; + + // Used as argument to an overloaded function with const and non-const. + T np_local2 = 42; + overloaded_arguments(np_local2); + + int np_local4 = 42; + // non-template values are ok still. + int p_local0 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const + np_local4 += p_local0; +}