Index: clang-tools-extra/clang-tidy/boost/BoostTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/boost/BoostTidyModule.cpp +++ clang-tools-extra/clang-tidy/boost/BoostTidyModule.cpp @@ -9,6 +9,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "UseRangeBasedForLoopCheck.h" #include "UseToStringCheck.h" using namespace clang::ast_matchers; @@ -19,6 +20,8 @@ class BoostModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "boost-use-range-based-for-loop"); CheckFactories.registerCheck("boost-use-to-string"); } }; Index: clang-tools-extra/clang-tidy/boost/CMakeLists.txt =================================================================== --- clang-tools-extra/clang-tidy/boost/CMakeLists.txt +++ clang-tools-extra/clang-tidy/boost/CMakeLists.txt @@ -5,6 +5,7 @@ add_clang_library(clangTidyBoostModule BoostTidyModule.cpp + UseRangeBasedForLoopCheck.cpp UseToStringCheck.cpp LINK_LIBS Index: clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.h @@ -0,0 +1,74 @@ +//===--- UseRangeBasedForLoopCheck.h - clang-tidy ---------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BOOST_USERANGEBASEDFORLOOPCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BOOST_USERANGEBASEDFORLOOPCHECK_H + +#include "../ClangTidyCheck.h" +#include +#include + +namespace clang { +namespace tidy { +namespace boost { + +/// Converts ``BOOST_FOREACH(..., ...)`` loops to use the new +/// range-based loops in C++11. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/boost-use-range-based-for-loop.html +class UseRangeBasedForLoopCheck : public ClangTidyCheck { +public: + enum PPJournalRecordStatus { Unknown = 0, Bad, Good }; + struct PPJournalRecord { + SourceLocation MacroNameLocation; + SourceRange VariableRange; + SourceRange ColRange; + PPJournalRecordStatus VariableStatus = Unknown; + PPJournalRecordStatus ColStatus = Unknown; + + bool known() const { + return VariableStatus != Unknown && ColStatus != Unknown; + } + bool good() const { return VariableStatus == Good && ColStatus == Good; } + }; + struct PPJournal : std::vector { + using std::vector::vector; + size_t getUnknownCount() const { + return std::count_if(begin(), end(), + [](const auto &R) { return !R.known(); }); + } + }; + + using ClangTidyCheck::ClangTidyCheck; + ~UseRangeBasedForLoopCheck(); + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus11; + } + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + bool isGoodFirst(const ast_matchers::MatchFinder::MatchResult &Result, + const clang::DynTypedNode *Node); + bool isGoodSecond(const ast_matchers::MatchFinder::MatchResult &Result, + const clang::Expr *Node); + void emitDiagnostic(const PPJournalRecord &R, SourceManager &SM); + +private: + PPJournal MacroExpansions; +}; + +} // namespace boost +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BOOST_USERANGEBASEDFORLOOPCHECK_H Index: clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.cpp @@ -0,0 +1,214 @@ +//===--- UseRangeBasedForLoopCheck.cpp - clang-tidy -----------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "UseRangeBasedForLoopCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/MacroArgs.h" +#include "clang/Lex/PPCallbacks.h" +#include "clang/Lex/Preprocessor.h" +#include +#include +#include + +#define DEBUG_TYPE "UseRangeBasedForLoopCheck" +#include "llvm/Support/Debug.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace boost { + +namespace { +class UseRangeBasedForLoopCallback : public PPCallbacks { +public: + explicit UseRangeBasedForLoopCallback( + UseRangeBasedForLoopCheck::PPJournal *Journal) + : Journal(Journal) {} + + // Detects expansion of the BOOST_FOREACH macro + void MacroExpands(const Token &MacroNameToken, + const MacroDefinition &MacroDefinition, SourceRange Range, + const MacroArgs *Args) override { + const IdentifierInfo *NameIdentifierInfo = + MacroNameToken.getIdentifierInfo(); + if (!NameIdentifierInfo) + return; + if (NameIdentifierInfo->getName() != "BOOST_FOREACH" || !Args || + Args->getNumMacroArguments() < 2) + return; + const Token *VariableTokens = Args->getUnexpArgument(0); + const Token *ColTokens = Args->getUnexpArgument(1); + if (!VariableTokens || !ColTokens) + return; + + unsigned VariableNumToks = clang::MacroArgs::getArgLength(VariableTokens); + unsigned ColNumToks = clang::MacroArgs::getArgLength(ColTokens); + + assert(VariableNumToks != 0 && ColNumToks != 0); + SourceLocation Loc = MacroNameToken.getLocation(); + Journal->push_back( + {Loc, + SourceRange(VariableTokens->getLocation(), + VariableTokens[VariableNumToks - 1].getLastLoc()), + SourceRange(ColTokens->getLocation(), + ColTokens[ColNumToks - 1].getLastLoc())}); + } + +private: + UseRangeBasedForLoopCheck::PPJournal *Journal; +}; +} // namespace + +UseRangeBasedForLoopCheck::~UseRangeBasedForLoopCheck() { + size_t C = 0; + LLVM_DEBUG(if ((C = MacroExpansions.getUnknownCount()) != 0) { + llvm::errs() << C << " expansions left in unknown status\n"; + }); +} + +void UseRangeBasedForLoopCheck::registerPPCallbacks( + const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) { + PP->addPPCallbacks( + std::make_unique(&MacroExpansions)); +} + +void UseRangeBasedForLoopCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + varDecl(isExpandedFromMacro("BOOST_FOREACH")).bind("possible_first"), + this); + Finder->addMatcher( + expr(isExpandedFromMacro("BOOST_FOREACH")).bind("possible_first"), this); + Finder->addMatcher( + expr(isExpandedFromMacro("BOOST_FOREACH")).bind("possible_second"), this); +} + +void UseRangeBasedForLoopCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedFirst = static_cast(nullptr); + const auto *MatchedSecond = Result.Nodes.getNodeAs("possible_second"); + SourceManager &SM = *Result.SourceManager; + + for (const auto &KeyValue : Result.Nodes.getMap()) { + if (KeyValue.first == "possible_first") { + MatchedFirst = &KeyValue.second; + break; + } + } + + for (auto &R : MacroExpansions) { + if (MatchedFirst) { + SourceRange Range = MatchedFirst->getSourceRange(); + SourceLocation FileLocStart = SM.getFileLoc(Range.getBegin()), + FileLocEnd = SM.getFileLoc(Range.getEnd()); + + if (R.VariableRange == SourceRange(FileLocStart, FileLocEnd) && + R.VariableStatus == Unknown) { + R.VariableStatus = isGoodFirst(Result, MatchedFirst) ? Good : Bad; + if (R.known()) { + emitDiagnostic(R, SM); + } + break; + } + } + if (MatchedSecond) { + SourceRange Range = MatchedSecond->getSourceRange(); + SourceLocation FileLocStart = SM.getFileLoc(Range.getBegin()), + FileLocEnd = SM.getFileLoc(Range.getEnd()); + + if (R.ColRange == SourceRange(FileLocStart, FileLocEnd) && + R.ColStatus == Unknown) { + R.ColStatus = isGoodSecond(Result, MatchedSecond) ? Good : Bad; + if (R.known()) { + emitDiagnostic(R, SM); + } + break; + } + } + } +} + +bool UseRangeBasedForLoopCheck::isGoodFirst( + const MatchFinder::MatchResult &Result, const clang::DynTypedNode *Node) { + return (Node->get() != nullptr); +} + +bool UseRangeBasedForLoopCheck::isGoodSecond( + const MatchFinder::MatchResult &Result, const clang::Expr *Node) { + // Test that the incoming type has a record declaration that has methods + // called 'begin' and 'end'. If the incoming type is const, then make sure + // these methods are also marked const. + // + // FIXME: To be completely thorough this matcher should also ensure the + // return type of begin/end is an iterator that dereferences to the same as + // what operator[] or at() returns. Such a test isn't likely to fail except + // for pathological cases. + // + // FIXME: Also, a record doesn't necessarily need begin() and end(). Free + // functions called begin() and end() taking the container as an argument + // are also allowed. + TypeMatcher RecordWithBeginEnd = qualType(anyOf( + qualType( + isConstQualified(), + hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl( + hasMethod(cxxMethodDecl(hasName("begin"), isConst())), + hasMethod(cxxMethodDecl(hasName("end"), + isConst())))) // hasDeclaration + ))), // qualType + qualType(unless(isConstQualified()), + hasUnqualifiedDesugaredType(recordType(hasDeclaration( + cxxRecordDecl(hasMethod(hasName("begin")), + hasMethod(hasName("end"))))))) // qualType + )); + + // Test that the incoming type is a c-style array. + TypeMatcher Array = arrayType(); + + if (selectFirst( + "val", match(expr(anyOf(hasType(RecordWithBeginEnd), hasType(Array))) + .bind("val"), + *Node, *Result.Context)) != nullptr) + return true; + return false; +} + +void UseRangeBasedForLoopCheck::emitDiagnostic(const PPJournalRecord &R, + SourceManager &SM) { + auto Diagnostic = diag(R.MacroNameLocation, + "use range-based for loop instead of BOOST_FOREACH"); + + if (R.good()) { + SourceLocation Loc = R.MacroNameLocation; + const char *Data = SM.getCharacterData(SM.getExpansionLoc(Loc)); + const char *End = SM.getCharacterData(SM.getExpansionLoc(Loc)); + while (isAsciiIdentifierContinue(*End)) + ++End; + unsigned Len = std::distance(Data, End); + + SourceLocation FirstEnd = R.VariableRange.getEnd().getLocWithOffset(1); + const char *AfterFirstData = SM.getCharacterData(FirstEnd); + unsigned AfterFirstLen = std::strcspn(AfterFirstData, ","); + + Diagnostic << FixItHint::CreateReplacement( + CharSourceRange::getCharRange( + SM.getExpansionLoc(Loc), + SM.getExpansionLoc(Loc).getLocWithOffset(Len)), + "for"); + Diagnostic << FixItHint::CreateReplacement( + CharSourceRange::getCharRange( + FirstEnd.getLocWithOffset(AfterFirstLen), + FirstEnd.getLocWithOffset(AfterFirstLen + 1)), + ":"); + } +} + +} // namespace boost +} // namespace tidy +} // namespace clang Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -103,6 +103,12 @@ Finds initializations of C++ shared pointers to non-array type that are initialized with an array. +- New :doc:`boost-use-range-based-for-loop + ` check. + + This check converts ``BOOST_FOREACH(..., ...)`` loops to use the new + range-based loops in C++11. + New check aliases ^^^^^^^^^^^^^^^^^ Index: clang-tools-extra/docs/clang-tidy/checks/boost-use-range-based-for-loop.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/boost-use-range-based-for-loop.rst @@ -0,0 +1,91 @@ +.. title:: clang-tidy - boost-use-range-based-for-loop + +boost-use-range-based-for-loop +============================== + +This check converts ``BOOST_FOREACH(..., ...)`` loops to use the new +range-based loops in C++11. + +Two kinds of loops can be converted: + +- Loops over arrays. +- Loops over STL containers and other user defined array-like containers. + +Before: + +.. code-block:: c++ + + std::list list_int( /*...*/ ); + BOOST_FOREACH( int i, list_int ) + { + // do something with i + } + +After: + +.. code-block:: c++ + + std::list list_int( /*...*/ ); + for( int i: list_int ) + { + // do something with i + } + +Known Limitations +----------------- +* This check won't find ``BOOST_REVERSE_FOREACH(..., ...)`` loops. + +* User defined array-like containers must have ``begin`` and ``end`` methods. + This check cannot migrate containers with only free-standing + ``begin`` and ``end`` functions. + +* Client code that use ``BOOST_FOREACH`` with: + + - ``std::pair`` of iterators; + - Null-terminated strings (``char`` and ``wchar_t``); + - `Extensions of Boost.Foreach `_; + - Some other undefined sequence types + +won't be migrated. Please see list of correctly convertible loops above. + +.. code-block:: c++ + + const char* cstr = "Hello world"; + BOOST_FOREACH( char c, cstr ) + { + // do something with c + } + + // Won't change, just warning + +* Range based for loop(unlike BOOST_FOREACH) does not understand `Extensions of Boost.Range `_. + In some cases behaviour might be changed or broken code might be produced. + +* Second argument of the ``BOOST_FOREACH`` macro must be independent of the + template parameters, all other won't be migrated. + +* First argument of the ``BOOST_FOREACH`` macro must be only new identifier + definition, all other won't be migrated. + +.. code-block:: c++ + + std::list list_int( /*...*/ ); + int i; + BOOST_FOREACH( i, list_int ) + { + // do something with i + } + + // Won't change, just warning + +* ``BOOST_FOREACH`` macro should not contain some another macro expansions, and should not be in macro body. Otherwise, it will not be detected. + +.. code-block:: c++ + + std::list> list_of_maps( /*...*/ ); + BOOST_FOREACH(BOOST_IDENTITY_TYPE(std::map) m, list_of_maps) + { + // do something with m + } + + // No warning, no migration Index: clang-tools-extra/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -51,6 +51,7 @@ `android-cloexec-pipe2 `_, `android-cloexec-socket `_, `android-comparison-in-temp-failure-retry `_, + `boost-use-range-based-for-loop `_, "Yes" `boost-use-to-string `_, "Yes" `bugprone-argument-comment `_, "Yes" `bugprone-assert-side-effect `_, Index: clang-tools-extra/test/clang-tidy/checkers/boost-use-range-based-for-loop.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/boost-use-range-based-for-loop.cpp @@ -0,0 +1,229 @@ +// RUN: %check_clang_tidy %s boost-use-range-based-for-loop %t + +namespace std { +template +class list { +public: + using iterator = T*; + using value_type = T; + iterator begin(); + iterator end(); +}; +template +class map {}; +template +class tuple {}; +template +struct pair { + explicit pair(const First&, const Second&); +}; +} + +#define BOOST_FOREACH(VAR, COL) for(VAR;(COL, false);) +#define IDENTITY_TYPE(...) __VA_ARGS__ + +void test_range_based_for_loop() +{ + std::list list_int; + BOOST_FOREACH( int i, list_int ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead of BOOST_FOREACH [boost-use-range-based-for-loop] + // CHECK-FIXES: for( int i: list_int ) { + + short array_short[] = {1,2,3}; + BOOST_FOREACH(int val, array_short) { + /// The short was implicitly converted to an int + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for(int val: array_short) { +} + +#define foreach_ BOOST_FOREACH +#define foreach_2nd_ foreach_ + +void test_range_based_for_loop_prettier() +{ + std::list l; + foreach_( int i, l ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for( int i: l ) { + + short array_short[] = {1,2,3}; + foreach_2nd_(int val, array_short) { + /// The short was implicitly converted to an int + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for(int val: array_short) { +} + +template +void test_range_based_for_loop_in_template_instantiation(const std::list& some_list + , const Containter& some_containter) +{ + std::list list_int; + BOOST_FOREACH( int i, list_int ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead of BOOST_FOREACH [boost-use-range-based-for-loop] + // CHECK-FIXES: for( int i: list_int ) { + + // FIXME +#if 0 + BOOST_FOREACH( T i, some_list ) { + // do something with i + } + // :[[@LINE-3]]:3: warning: use range-based for loop instead of BOOST_FOREACH [boost-use-range-based-for-loop] + // for( T i: some_list ) { +#endif + + BOOST_FOREACH( typename Containter::value_type i, some_containter ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead of BOOST_FOREACH [boost-use-range-based-for-loop] + // CHECK-FIXES: BOOST_FOREACH( typename Containter::value_type i, some_containter ) { + // ^ so no fix-its here. +} + +void test_range_based_for_loop_without_spaces() +{ + std::list> l; + BOOST_FOREACH(std::listi,l){ + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for(std::listi:l){ +} + +void test_range_based_for_loop_no_migration() +{ + // FIXME: implement these migrations and remove here tests + std::list l; + using rng = std::pair::iterator, std::list::iterator>; + + int i; + BOOST_FOREACH( i, l ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: BOOST_FOREACH( i, l ) { + // ^ so no fix-its here. + + BOOST_FOREACH( int val, rng(l.begin(), l.end()) ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: BOOST_FOREACH( int val, rng(l.begin(), l.end()) ) { + // ^ so no fix-its here. +} + +void test_range_based_for_loop_keep_comments() +{ + std::list l; + std::list l2; + + /* 1 */ BOOST_FOREACH( int i, l=l2 ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:11: warning: use range-based + // CHECK-FIXES: /* 1 */ for( int i: l=l2 ) { + + BOOST_FOREACH /* 1 */ ( int i, l=l2 ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for /* 1 */ ( int i: l=l2 ) { + + BOOST_FOREACH( /* 1 */ int i, l=l2 ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for( /* 1 */ int i: l=l2 ) { + + BOOST_FOREACH( int /* 1 */ i, l=l2 ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for( int /* 1 */ i: l=l2 ) { + + BOOST_FOREACH( int i /* 1 */, l=l2 ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for( int i /* 1 */: l=l2 ) { + + BOOST_FOREACH( int i, /* 1 */ l=l2 ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for( int i: /* 1 */ l=l2 ) { + + BOOST_FOREACH( int i, l /* 1 */ =l2 ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for( int i: l /* 1 */ =l2 ) { + + BOOST_FOREACH( int i, l= /* 1 */ l2 ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for( int i: l= /* 1 */ l2 ) { + + BOOST_FOREACH( int i, l=l2 /* 1 */ ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for( int i: l=l2 /* 1 */ ) { + + BOOST_FOREACH( int i, l=l2 ) /* 1 */ { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based + // CHECK-FIXES: for( int i: l=l2 ) /* 1 */ { +} + +// FIXME: implement this case and revise the relevant limitation about macro in the doc +#if 0 +void test_range_based_for_loop_comma_in_first_arg() +{ + std::list> list_of_maps; + BOOST_FOREACH(IDENTITY_TYPE(std::map) m, list_of_maps) { + // do something with m + } + // :[[@LINE-3]]:3: warning: use range-based + / for(IDENTITY_TYPE(std::map) m: list_of_maps) { + + std::list> list_of_tuples; + BOOST_FOREACH(IDENTITY_TYPE(std::tuple) t, list_of_tuples) { + // do something with t + } + // :[[@LINE-3]]:3: warning: use range-based + // for(IDENTITY_TYPE(std::tuple) t: list_of_tuples) { +} +#endif + +void test_range_based_for_loop_multiline_args() +{ + std::list l; + std::list l2; + BOOST_FOREACH( + int + i + , + l + = + l2 ) { + // do something with i + } + // CHECK-MESSAGES: :[[@LINE-9]]:3: warning: use range-based + // CHECK-FIXES: for( + // CHECK-FIXES: int + // CHECK-FIXES: i + // CHECK-FIXES: : + // CHECK-FIXES: l + // CHECK-FIXES: = + // CHECK-FIXES: l2 ) { +}