diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.h --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.h +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.h @@ -10,6 +10,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDNONCONSTGLOBALVARIABLESCHECK_H #include "../ClangTidyCheck.h" +#include namespace clang { namespace tidy { @@ -21,6 +22,18 @@ /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.html class AvoidNonConstGlobalVariablesCheck : public ClangTidyCheck { + std::string generateReplacementString( + const ast_matchers::MatchFinder::MatchResult &Result, + const VarDecl &Variablee) const; + + bool hasSpaceAfterType(const ast_matchers::MatchFinder::MatchResult &Result, + const VarDecl &Variable, + const std::string &NonConstType) const; + + CharSourceRange generateReplacementRange(const VarDecl &Variable) const; + + std::string cleanType(const QualType &Type) const; + public: AvoidNonConstGlobalVariablesCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context) {} diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp @@ -10,6 +10,8 @@ #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Lex/Lexer.h" +#include using namespace clang::ast_matchers; @@ -49,9 +51,15 @@ if (const auto *Variable = Result.Nodes.getNodeAs("non-const_variable")) { + + auto ReplacementRange = generateReplacementRange(*Variable); + auto Replacement = generateReplacementString(Result, *Variable); + diag(Variable->getLocation(), "variable %0 is non-const and globally " "accessible, consider making it const") - << Variable; // FIXME: Add fix-it hint to Variable + << Variable + << FixItHint::CreateReplacement(ReplacementRange, Replacement); + // Don't return early, a non-const variable may also be a pointer or // reference to non-const data. } @@ -61,11 +69,89 @@ diag(VD->getLocation(), "variable %0 provides global access to a non-const object; consider " "making the %select{referenced|pointed-to}1 data 'const'") - << VD - << VD->getType()->isPointerType(); // FIXME: Add fix-it hint to Variable + << VD << VD->getType()->isPointerType() + << FixItHint::CreateInsertion(VD->getBeginLoc(), "const "); } } +/// Makes the provided type constant and converts it to a string. +/// If the current type sticks to the variable name as in the example below, a +/// space is inserted: +// +/// \code +/// int *y = &x; +/// ^^ +/// \endcode +/// +/// \returns string representation of the constant type of \p Variable. +std::string AvoidNonConstGlobalVariablesCheck::generateReplacementString( + const MatchFinder::MatchResult &Result, const VarDecl &Variable) const { + + auto Type = Variable.getType(); + bool HasSpace = hasSpaceAfterType(Result, Variable, cleanType(Type)); + Type.addConst(); + return cleanType(Type) + (HasSpace ? "" : " "); +} + +bool AvoidNonConstGlobalVariablesCheck::hasSpaceAfterType( + const MatchFinder::MatchResult &Result, const VarDecl &Variable, + const std::string &NonConstType) const { + + StringRef VariableText = Lexer::getSourceText( + CharSourceRange::getTokenRange(Variable.getSourceRange()), + *Result.SourceManager, getLangOpts()); + + /// Check to make the function error-robust in case \c NonConstType.length() + /// exceeds \c VariableText length. This would occure if the \c PrintingPolicy + /// used in \c printCleanedType did not remove all superfluous type + /// information. As a fallback, it is assumed that the type is not followed by + /// a space in the source code. + if (NonConstType.length() > VariableText.str().length()) { + llvm::errs() << "Checking for space failed: the type's effective " + "length is greater than the variable declaration."; + return false; + } + + return std::isspace(VariableText.str().at(NonConstType.length())); +} + +CharSourceRange AvoidNonConstGlobalVariablesCheck::generateReplacementRange( + const VarDecl &Variable) const { + + auto TypeBeginLoc = Variable.getBeginLoc(); + + auto TypeEndLoc = + TypeBeginLoc.getLocWithOffset(cleanType(Variable.getType()).length()); + + return CharSourceRange::getCharRange(TypeBeginLoc, TypeEndLoc); +} + +/// Creates a string representation of \p Type and suppresses the \c class +/// keyword in a class instance's type. Also, on unnamed/anonymous types, the +/// tag location and the \c unnamed or \c anonymous keyword are removed from the +/// type description. If this would not be done, those keywords would be +/// inserted into the source code as part of the \c FixItHint replacement. +std::string +AvoidNonConstGlobalVariablesCheck::cleanType(const QualType &Type) const { + + /// \c PrintingPolicy suppresses the "class" keyword in a class + /// instance's type and locations of anonymous tags. + PrintingPolicy PrintingPolicy{getLangOpts()}; + PrintingPolicy.AnonymousTagLocations = false; + std::string PolicyCleanedType = Type.getAsString(PrintingPolicy); + + auto StringsToErase = SmallVector{" (unnamed)", " (anonymous)"}; + + for (std::string StringToErase : StringsToErase) { + size_t StartPositionToErase = PolicyCleanedType.find(StringToErase); + if (StartPositionToErase == std::string::npos) + continue; + PolicyCleanedType.erase(StartPositionToErase, StringToErase.length()); + } + + return PolicyCleanedType; +} + } // namespace cppcoreguidelines } // namespace tidy } // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -118,6 +118,12 @@ function or assignment to ``nullptr``. Added support for pointers to ``std::unique_ptr``. +- Improved :doc:`cppcoreguidelines-avoid-non-const-global-variables +