Index: clang-tidy/readability/CMakeLists.txt =================================================================== --- clang-tidy/readability/CMakeLists.txt +++ clang-tidy/readability/CMakeLists.txt @@ -3,6 +3,7 @@ add_clang_library(clangTidyReadabilityModule AvoidConstParamsInDecls.cpp BracesAroundStatementsCheck.cpp + ConstReturnTypeCheck.cpp ContainerSizeEmptyCheck.cpp DeleteNullPointerCheck.cpp DeletedDefaultCheck.cpp Index: clang-tidy/readability/ConstReturnTypeCheck.h =================================================================== --- /dev/null +++ clang-tidy/readability/ConstReturnTypeCheck.h @@ -0,0 +1,35 @@ +//===--- ConstReturnTypeCheck.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_READABILITY_CONSTRETURNTYPECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONSTRETURNTYPECHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// For any function whose return type is const-qualified, suggests removal of +/// the `const` qualifier from that return type. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability-const-return-type.html +class ConstReturnTypeCheck : public ClangTidyCheck { + public: + using ClangTidyCheck::ClangTidyCheck; + void registerMatchers(ast_matchers::MatchFinder* finder) override; + void check(const ast_matchers::MatchFinder::MatchResult& result) override; +}; + +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONSTRETURNTYPECHECK_H Index: clang-tidy/readability/ConstReturnTypeCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/readability/ConstReturnTypeCheck.cpp @@ -0,0 +1,128 @@ +//===--- ConstReturnTypeCheck.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 "ConstReturnTypeCheck.h" +#include "../utils/LexerUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/Optional.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace readability { + +// Finds the location of the qualifying `const` token in the `FunctionDecl`'s +// return type. Returns `None` when the return type is not `const`-qualified or +// `const` does not appear in `Def`'s source like when the type is an alias or a +// macro. +static llvm::Optional +findConstToRemove(const FunctionDecl *Def, + const MatchFinder::MatchResult &Result) { + if (!Def->getReturnType().isLocalConstQualified()) + return None; + + // Get the begin location for the function name, including any qualifiers + // written in the source (for out-of-line declarations). A FunctionDecl's + // "location" is the start of its name, so, when the name is unqualified, we + // use `getLocation()`. + SourceLocation NameBeginLoc = Def->getQualifier() + ? Def->getQualifierLoc().getBeginLoc() + : Def->getLocation(); + // Since either of the locs can be in a macro, use `makeFileCharRange` to be + // sure that we have a consistent `CharSourceRange`, located entirely in the + // source file. + CharSourceRange FileRange = Lexer::makeFileCharRange( + CharSourceRange::getCharRange(Def->getBeginLoc(), NameBeginLoc), + *Result.SourceManager, Result.Context->getLangOpts()); + + if (FileRange.isInvalid()) + return None; + + return utils::lexer::getConstQualifyingToken(FileRange, *Result.Context, + *Result.SourceManager); +} + +namespace { + +struct CheckResult { + // Source range of the relevant `const` token in the definition being checked. + CharSourceRange ConstRange; + + // FixItHints associated with the definition being checked. + llvm::SmallVector Hints; + + // Locations of any declarations that could not be fixed. + llvm::SmallVector DeclLocs; +}; + +} // namespace + +// Does the actual work of the check. +static CheckResult checkDef(const clang::FunctionDecl *Def, + const MatchFinder::MatchResult &MatchResult) { + CheckResult Result; + llvm::Optional Tok = findConstToRemove(Def, MatchResult); + if (!Tok) + return Result; + + Result.ConstRange = + CharSourceRange::getCharRange(Tok->getLocation(), Tok->getEndLoc()); + Result.Hints.push_back(FixItHint::CreateRemoval(Result.ConstRange)); + + // Fix the definition and any visible declarations, but don't warn + // seperately for each declaration. Instead, associate all fixes with the + // single warning at the definition. + for (const FunctionDecl *Decl = Def->getPreviousDecl(); Decl != nullptr; + Decl = Decl->getPreviousDecl()) { + if (llvm::Optional T = findConstToRemove(Decl, MatchResult)) + Result.Hints.push_back(FixItHint::CreateRemoval( + CharSourceRange::getCharRange(T->getLocation(), T->getEndLoc()))); + else + // `getInnerLocStart` gives the start of the return type. + Result.DeclLocs.push_back(Decl->getInnerLocStart()); + } + return Result; +} + +void ConstReturnTypeCheck::registerMatchers(MatchFinder *Finder) { + // Find all function definitions for which the return types are `const` + // qualified. + Finder->addMatcher( + functionDecl(returns(isConstQualified()), isDefinition()).bind("func"), + this); +} + +void ConstReturnTypeCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Def = Result.Nodes.getNodeAs("func"); + CheckResult CR = checkDef(Def, Result); + { + // Clang only supports one in-flight diagnostic at a time. So, delimit the + // scope of `Diagnostic` to allow further diagnostics after the scope. We + // use `getInnerLocStart` to get the start of the return type. + DiagnosticBuilder Diagnostic = + diag(Def->getInnerLocStart(), + "return type %0 is 'const'-qualified at the top level, which may " + "reduce code readability without improving const correctness") + << Def->getReturnType(); + if (CR.ConstRange.isValid()) + Diagnostic << CR.ConstRange; + for (auto &Hint : CR.Hints) + Diagnostic << Hint; + } + for (auto Loc : CR.DeclLocs) + diag(Loc, "could not transform this declaration", DiagnosticIDs::Note); +} + +} // namespace readability +} // namespace tidy +} // namespace clang Index: clang-tidy/readability/ReadabilityTidyModule.cpp =================================================================== --- clang-tidy/readability/ReadabilityTidyModule.cpp +++ clang-tidy/readability/ReadabilityTidyModule.cpp @@ -12,6 +12,7 @@ #include "../ClangTidyModuleRegistry.h" #include "AvoidConstParamsInDecls.h" #include "BracesAroundStatementsCheck.h" +#include "ConstReturnTypeCheck.h" #include "ContainerSizeEmptyCheck.h" #include "DeleteNullPointerCheck.h" #include "DeletedDefaultCheck.h" @@ -52,6 +53,8 @@ "readability-avoid-const-params-in-decls"); CheckFactories.registerCheck( "readability-braces-around-statements"); + CheckFactories.registerCheck( + "readability-const-return-type"); CheckFactories.registerCheck( "readability-container-size-empty"); CheckFactories.registerCheck( Index: clang-tidy/utils/LexerUtils.h =================================================================== --- clang-tidy/utils/LexerUtils.h +++ clang-tidy/utils/LexerUtils.h @@ -82,6 +82,14 @@ const SourceManager &SM, const LangOptions &LangOpts); +/// Assuming that ``Range`` spans a const-qualified type, returns the ``const`` +/// token in ``Range`` that is responsible for const qualification. ``Range`` +/// must be valid with respect to ``SM``. Returns ``None`` if no ``const`` +/// tokens are found. +llvm::Optional getConstQualifyingToken(CharSourceRange Range, + const ASTContext &Context, + const SourceManager &SM); + } // namespace lexer } // namespace utils } // namespace tidy Index: clang-tidy/utils/LexerUtils.cpp =================================================================== --- clang-tidy/utils/LexerUtils.cpp +++ clang-tidy/utils/LexerUtils.cpp @@ -92,6 +92,34 @@ return false; } + +llvm::Optional getConstQualifyingToken(CharSourceRange Range, + const ASTContext &Context, + const SourceManager &SM) { + std::pair LocInfo = SM.getDecomposedLoc(Range.getBegin()); + StringRef File = SM.getBufferData(LocInfo.first); + Lexer RawLexer(SM.getLocForStartOfFile(LocInfo.first), Context.getLangOpts(), + File.begin(), File.data() + LocInfo.second, File.end()); + llvm::Optional FirstConstTok; + Token LastTokInRange; + Token Tok; + while (!RawLexer.LexFromRawLexer(Tok) && + Range.getEnd() != Tok.getLocation() && + !SM.isBeforeInTranslationUnit(Range.getEnd(), Tok.getLocation())) { + if (Tok.is(tok::raw_identifier)) { + IdentifierInfo &Info = Context.Idents.get( + StringRef(SM.getCharacterData(Tok.getLocation()), Tok.getLength())); + Tok.setIdentifierInfo(&Info); + Tok.setKind(Info.getTokenID()); + } + if (Tok.is(tok::kw_const) && !FirstConstTok) + FirstConstTok = Tok; + LastTokInRange = Tok; + } + // If the last token in the range is a `const`, then it const qualifies the + // type. Otherwise, the first `const` token, if any, is the qualifier. + return LastTokInRange.is(tok::kw_const) ? LastTokInRange : FirstConstTok; +} } // namespace lexer } // namespace utils } // namespace tidy Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -142,6 +142,12 @@ Detects local variable declarations declaring more than one variable and tries to refactor the code to one statement per declaration. +- New :doc:`readability-const-return-type + ` check. + + Checks for functions with a ``const``-qualified return type and recommends + removal of the ``const`` keyword. + - New :doc:`readability-magic-numbers ` check. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -225,6 +225,7 @@ portability-simd-intrinsics readability-avoid-const-params-in-decls readability-braces-around-statements + readability-const-return-type readability-container-size-empty readability-delete-null-pointer readability-deleted-default Index: docs/clang-tidy/checks/readability-const-return-type.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/readability-const-return-type.rst @@ -0,0 +1,26 @@ +.. title:: clang-tidy - readability-const-return-type + +readability-const-return-type +============================= + +Checks for functions with a ``const``-qualified return type and recommends +removal of the ``const`` keyword. Such use of `const` is usually superfluous, +and can prevent valuable compiler optimizations. Does not (yet) fix trailing +return types. + +Examples: + +.. code-block:: c++ + + const int foo(); + const Clazz foo(); + Clazz *const foo(); + +Note that this applies strictly to top-level qualification, which excludes +pointers or references to const values. For example, these are fine: + +.. code-block:: c++ + + const int* foo(); + const int& foo(); + const Clazz* foo(); Index: test/clang-tidy/readability-const-return-type.cpp =================================================================== --- /dev/null +++ test/clang-tidy/readability-const-return-type.cpp @@ -0,0 +1,227 @@ +// RUN: %check_clang_tidy %s readability-const-return-type %t -- -- -isystem + +// p# = positive test +// n# = negative test + +#include + +const int p1() { +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness +// CHECK-FIXES: int p1() { + return 1; +} + +const int p15(); +// CHECK-FIXES: int p15(); + +template +const int p31(T v) { return 2; } +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu +// CHECK-FIXES: int p31(T v) { return 2; } + +// We detect const-ness even without instantiating T. +template +const T p32(T t) { return t; } +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const T' is 'const'-qual +// CHECK-FIXES: T p32(T t) { return t; } + +// However, if the return type is itself a template instantiation, Clang does +// not consider it const-qualified without knowing `T`. +template +typename std::add_const::type n15(T v) { return v; } + +template +class Klazz { +public: + Klazz(A) {} +}; + +class Clazz { + public: + Clazz *const p2() { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'Clazz *const' is 'co + // CHECK-FIXES: Clazz *p2() { + return this; + } + + Clazz *const p3(); + // CHECK-FIXES: Clazz *p3(); + + const int p4() const { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const + // CHECK-FIXES: int p4() const { + return 4; + } + + const Klazz* const p5() const; + // CHECK-FIXES: const Klazz* p5() const; + + const Clazz operator++(int x) { // p12 + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const Clazz' is 'const + // CHECK-FIXES: Clazz operator++(int x) { + } + + struct Strukt { + int i; + }; + + const Strukt p6() {} + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const Clazz::Strukt' i + // CHECK-FIXES: Strukt p6() {} + + // No warning is emitted here, because this is only the declaration. The + // warning will be associated with the definition, below. + const Strukt* const p7(); + // CHECK-FIXES: const Strukt* p7(); + + // const-qualifier is the first `const` token, but not the first token. + static const int p8() {} + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'- + // CHECK-FIXES: static int p8() {} + + static const Strukt p9() {} + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const Clazz::Strukt' i + // CHECK-FIXES: static Strukt p9() {} + + int n0() const { return 0; } + const Klazz& n11(const Klazz) const; +}; + +Clazz *const Clazz::p3() { + // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'Clazz *const' is 'cons + // CHECK-FIXES: Clazz *Clazz::p3() { + return this; +} + +const Klazz* const Clazz::p5() const {} +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz * +// CHECK-FIXES: const Klazz* Clazz::p5() const {} + +const Clazz::Strukt* const Clazz::p7() {} +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Clazz::Strukt *con +// CHECK-FIXES: const Clazz::Strukt* Clazz::p7() {} + +Clazz *const p10(); +// CHECK-FIXES: Clazz *p10(); + +Clazz *const p10() { + // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'Clazz *const' is 'cons + // CHECK-FIXES: Clazz *p10() { + return new Clazz(); +} + +const Clazz bar; +const Clazz *const p11() { + // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Clazz *const' is + // CHECK-FIXES: const Clazz *p11() { + return &bar; +} + +const Klazz p12() {} +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz' +// CHECK-FIXES: Klazz p12() {} + +const Klazz* const p13() {} +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz * +// CHECK-FIXES: const Klazz* p13() {} + +// re-declaration of p15. +const int p15(); +// CHECK-FIXES: int p15(); + +const int p15() { +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: +// CHECK-FIXES: int p15() { + return 0; +} + +// Exercise the lexer. + +const /* comment */ /* another comment*/ int p16() { return 0; } +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: +// CHECK-FIXES: /* comment */ /* another comment*/ int p16() { return 0; } + +/* comment */ const +// CHECK-MESSAGES: [[@LINE-1]]:15: warning: +// CHECK-FIXES: /* comment */ +// more +/* another comment*/ int p17() { return 0; } + +// Test cases where the `const` token lexically is hidden behind some form of +// indirection. + +#define CONSTINT const int +CONSTINT p18() {} +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu + +#define CONST const +CONST int p19() {} +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu + +using ty = const int; +ty p21() {} +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'ty' (aka 'const int') is + +typedef const int ty2; +ty2 p22() {} +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'ty2' (aka 'const int') i + +// Declaration uses a macro, while definition doesn't. In this case, we won't +// fix the declaration, and will instead issue a warning. +CONST int p23(); +// CHECK-NOTE: [[@LINE-1]]:1: note: could not transform this declaration + +const int p23(); +// CHECK-FIXES: int p23(); + +const int p23() { return 3; } +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu +// CHECK-FIXES: int p23() { return 3; } + +int const p24() { return 3; } +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu +// CHECK-FIXES: int p24() { return 3; } + +int const * const p25(const int* p) { return p; } +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int *const' is 'co +// CHECK-FIXES: int const * p25(const int* p) { return p; } + +// We cannot (yet) fix instances that use trailing return types, but we can +// warn. +auto p26() -> const int { return 3; } +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu +auto p27() -> int const { return 3; } +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu + +std::add_const::type p28() { return 3; } +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'std::add_const::typ + +// p29, p30 are based on +// llvm/projects/test-suite/SingleSource/Benchmarks/Misc-C++-EH/spirit.cpp: +template +Klazz const p29(T const &t) { return {}; } +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz' is +// CHECK-FIXES: Klazz p29(T const &t) { return {}; } + +Klazz const p30(char const *s) { return s; } +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz p30(char const *s) { return s; } + +const int n1 = 1; +const Clazz n2 = Clazz(); +const Clazz* n3 = new Clazz(); +Clazz *const n4 = new Clazz(); +const Clazz *const n5 = new Clazz(); +constexpr int n6 = 6; +constexpr int n7() { return 8; } +const int eight = 8; +constexpr const int* n8() { return &eight; } +Klazz n9(); +const Klazz* n10(); +const Klazz& Clazz::n11(const Klazz) const {} + +// Declaration only. +const int n14(); + +int **const * n_multiple_ptr(); +int *const & n_pointer_ref();