Index: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt +++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt @@ -7,6 +7,7 @@ BoolPointerImplicitConversionCheck.cpp DanglingHandleCheck.cpp DefinitionsInHeadersCheck.cpp + FoldInitTypeCheck.cpp ForwardDeclarationNamespaceCheck.cpp InaccurateEraseCheck.cpp IncorrectRoundings.cpp Index: clang-tools-extra/trunk/clang-tidy/misc/FoldInitTypeCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/FoldInitTypeCheck.h +++ clang-tools-extra/trunk/clang-tidy/misc/FoldInitTypeCheck.h @@ -0,0 +1,44 @@ +//===--- FoldInitTypeCheck.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_MISC_FOLD_INIT_TYPE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FOLD_INIT_TYPE_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// Find and flag invalid initializer values in folds, e.g. std::accumulate. +/// Example: +/// \code +/// auto v = {65536L * 65536 * 65536}; +/// std::accumulate(begin(v), end(v), 0 /* int type is too small */); +/// \endcode +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-fold-init-type.html +class FoldInitTypeCheck : public ClangTidyCheck { +public: + FoldInitTypeCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + void doCheck(const BuiltinType &IterValueType, const BuiltinType &InitType, + const ASTContext &Context, const CallExpr &CallNode); +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FOLD_INIT_TYPE_H Index: clang-tools-extra/trunk/clang-tidy/misc/FoldInitTypeCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/FoldInitTypeCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/misc/FoldInitTypeCheck.cpp @@ -0,0 +1,140 @@ +//===--- FoldInitTypeCheck.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 "FoldInitTypeCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +void FoldInitTypeCheck::registerMatchers(MatchFinder *Finder) { + // We match functions of interest and bind the iterator and init value types. + // Note: Right now we check only builtin types. + const auto BuiltinTypeWithId = [](const char *ID) { + return hasCanonicalType(builtinType().bind(ID)); + }; + const auto IteratorWithValueType = [&BuiltinTypeWithId](const char *ID) { + return anyOf( + // Pointer types. + pointsTo(BuiltinTypeWithId(ID)), + // Iterator types. + recordType(hasDeclaration(has(typedefNameDecl( + hasName("value_type"), hasType(BuiltinTypeWithId(ID))))))); + }; + + const auto IteratorParam = parmVarDecl( + hasType(hasCanonicalType(IteratorWithValueType("IterValueType")))); + const auto Iterator2Param = parmVarDecl( + hasType(hasCanonicalType(IteratorWithValueType("Iter2ValueType")))); + const auto InitParam = parmVarDecl(hasType(BuiltinTypeWithId("InitType"))); + + // std::accumulate, std::reduce. + Finder->addMatcher( + callExpr(callee(functionDecl( + hasAnyName("::std::accumulate", "::std::reduce"), + hasParameter(0, IteratorParam), hasParameter(2, InitParam))), + argumentCountIs(3)) + .bind("Call"), + this); + // std::inner_product. + Finder->addMatcher( + callExpr(callee(functionDecl(hasName("::std::inner_product"), + hasParameter(0, IteratorParam), + hasParameter(2, Iterator2Param), + hasParameter(3, InitParam))), + argumentCountIs(4)) + .bind("Call"), + this); + // std::reduce with a policy. + Finder->addMatcher( + callExpr(callee(functionDecl(hasName("::std::reduce"), + hasParameter(1, IteratorParam), + hasParameter(3, InitParam))), + argumentCountIs(4)) + .bind("Call"), + this); + // std::inner_product with a policy. + Finder->addMatcher( + callExpr(callee(functionDecl(hasName("::std::inner_product"), + hasParameter(1, IteratorParam), + hasParameter(3, Iterator2Param), + hasParameter(4, InitParam))), + argumentCountIs(5)) + .bind("Call"), + this); +} + +/// Returns true if ValueType is allowed to fold into InitType, i.e. if: +/// static_cast(ValueType{some_value}) +/// does not result in trucation. +static bool isValidBuiltinFold(const BuiltinType &ValueType, + const BuiltinType &InitType, + const ASTContext &Context) { + const auto ValueTypeSize = Context.getTypeSize(&ValueType); + const auto InitTypeSize = Context.getTypeSize(&InitType); + // It's OK to fold a float into a float of bigger or equal size, but not OK to + // fold into an int. + if (ValueType.isFloatingPoint()) + return InitType.isFloatingPoint() && InitTypeSize >= ValueTypeSize; + // It's OK to fold an int into: + // - an int of the same size and signedness. + // - a bigger int, regardless of signedness. + // - FIXME: should it be a warning to fold into floating point? + if (ValueType.isInteger()) { + if (InitType.isInteger()) { + if (InitType.isSignedInteger() == ValueType.isSignedInteger()) + return InitTypeSize >= ValueTypeSize; + return InitTypeSize > ValueTypeSize; + } + if (InitType.isFloatingPoint()) + return InitTypeSize >= ValueTypeSize; + } + return false; +} + +/// Prints a diagnostic if IterValueType doe snot fold into IterValueType (see +// isValidBuiltinFold for details). +void FoldInitTypeCheck::doCheck(const BuiltinType &IterValueType, + const BuiltinType &InitType, + const ASTContext &Context, + const CallExpr &CallNode) { + if (!isValidBuiltinFold(IterValueType, InitType, Context)) { + diag(CallNode.getExprLoc(), "folding type %0 into type %1 might result in " + "loss of precision") + << IterValueType.desugar() << InitType.desugar(); + } +} + +void FoldInitTypeCheck::check(const MatchFinder::MatchResult &Result) { + // Given the iterator and init value type retreived by the matchers, + // we check that the ::value_type of the iterator is compatible with + // the init value type. + const auto *InitType = Result.Nodes.getNodeAs("InitType"); + const auto *IterValueType = + Result.Nodes.getNodeAs("IterValueType"); + assert(InitType != nullptr); + assert(IterValueType != nullptr); + + const auto *CallNode = Result.Nodes.getNodeAs("Call"); + assert(CallNode != nullptr); + + doCheck(*IterValueType, *InitType, *Result.Context, *CallNode); + + if (const auto *Iter2ValueType = + Result.Nodes.getNodeAs("Iter2ValueType")) + doCheck(*Iter2ValueType, *InitType, *Result.Context, *CallNode); +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp +++ clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp @@ -16,6 +16,7 @@ #include "BoolPointerImplicitConversionCheck.h" #include "DanglingHandleCheck.h" #include "DefinitionsInHeadersCheck.h" +#include "FoldInitTypeCheck.h" #include "ForwardDeclarationNamespaceCheck.h" #include "InaccurateEraseCheck.h" #include "IncorrectRoundings.h" @@ -67,6 +68,8 @@ "misc-dangling-handle"); CheckFactories.registerCheck( "misc-definitions-in-headers"); + CheckFactories.registerCheck( + "misc-fold-init-type"); CheckFactories.registerCheck( "misc-forward-declaration-namespace"); CheckFactories.registerCheck( Index: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst @@ -53,6 +53,7 @@ misc-bool-pointer-implicit-conversion misc-dangling-handle misc-definitions-in-headers + misc-fold-init-type misc-forward-declaration-namespace misc-inaccurate-erase misc-incorrect-roundings Index: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-fold-init-type.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-fold-init-type.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-fold-init-type.rst @@ -0,0 +1,27 @@ +.. title:: clang-tidy - misc-fold-init-type + +misc-fold-init-type +=================== + +The check flags type mismatches in +`folds `_ +like `std::accumulate` that might result in loss of precision. +`std::accumulate` folds an input range into an initial value using the type of +the latter, with `operator+` by default. This can cause loss of precision +through: + +- Truncation: The following code uses a floating point range and an int + initial value, so trucation wil happen at every application of `operator+` + and the result will be `0`, which might not be what the user expected. + +.. code:: c++ + + auto a = {0.5f, 0.5f, 0.5f, 0.5f}; + return std::accumulate(std::begin(a), std::end(a), 0); + +- Overflow: The following code also returns `0`. + +.. code:: c++ + + auto a = {65536LL * 65536 * 65536}; + return std::accumulate(std::begin(a), std::end(a), 0); Index: clang-tools-extra/trunk/test/clang-tidy/misc-fold-init-type.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-fold-init-type.cpp +++ clang-tools-extra/trunk/test/clang-tidy/misc-fold-init-type.cpp @@ -0,0 +1,158 @@ +// RUN: %check_clang_tidy %s misc-fold-init-type %t + +namespace std { +template +T accumulate(InputIt first, InputIt last, T init); + +template +T reduce(InputIt first, InputIt last, T init); +template +T reduce(ExecutionPolicy &&policy, + InputIt first, InputIt last, T init); + +struct parallel_execution_policy {}; +constexpr parallel_execution_policy par{}; + +template +T inner_product(InputIt1 first1, InputIt1 last1, + InputIt2 first2, T value); + +template +T inner_product(ExecutionPolicy &&policy, InputIt1 first1, InputIt1 last1, + InputIt2 first2, T value); + +} // namespace std + +struct FloatIterator { + typedef float value_type; +}; +template +struct TypedefTemplateIterator { typedef ValueType value_type; }; +template +struct UsingTemplateIterator { using value_type = ValueType; }; +template +struct DependentTypedefTemplateIterator { typedef typename ValueType::value_type value_type; }; +template +struct DependentUsingTemplateIterator : public TypedefTemplateIterator { using typename TypedefTemplateIterator::value_type; }; +using TypedeffedIterator = FloatIterator; + +// Positives. + +int accumulatePositive1() { + float a[1] = {0.5f}; + return std::accumulate(a, a + 1, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int' +} + +int accumulatePositive2() { + FloatIterator it; + return std::accumulate(it, it, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int' +} + +int accumulatePositive3() { + double a[1] = {0.0}; + return std::accumulate(a, a + 1, 0.0f); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'double' into type 'float' +} + +int accumulatePositive4() { + TypedefTemplateIterator it; + return std::accumulate(it, it, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'unsigned int' into type 'int' +} + +int accumulatePositive5() { + UsingTemplateIterator it; + return std::accumulate(it, it, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'unsigned int' into type 'int' +} + +int accumulatePositive6() { + DependentTypedefTemplateIterator> it; + return std::accumulate(it, it, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'unsigned int' into type 'int' +} + +int accumulatePositive7() { + TypedeffedIterator it; + return std::accumulate(it, it, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int' +} + +int accumulatePositive8() { + DependentUsingTemplateIterator it; + return std::accumulate(it, it, 0); + // FIXME: this one should trigger too. +} + +int reducePositive1() { + float a[1] = {0.5f}; + return std::reduce(a, a + 1, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int' +} + +int reducePositive2() { + float a[1] = {0.5f}; + return std::reduce(std::par, a, a + 1, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int' +} + +int innerProductPositive1() { + float a[1] = {0.5f}; + int b[1] = {1}; + return std::inner_product(std::par, a, a + 1, b, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int' +} + +int innerProductPositive2() { + float a[1] = {0.5f}; + int b[1] = {1}; + return std::inner_product(std::par, a, a + 1, b, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int' +} + +// Negatives. + +int negative1() { + float a[1] = {0.5f}; + // This is OK because types match. + return std::accumulate(a, a + 1, 0.0); +} + +int negative2() { + float a[1] = {0.5f}; + // This is OK because double is bigger than float. + return std::accumulate(a, a + 1, 0.0); +} + +int negative3() { + float a[1] = {0.5f}; + // This is OK because the user explicitly specified T. + return std::accumulate(a, a + 1, 0); +} + +int negative4() { + TypedefTemplateIterator it; + // For now this is OK. + return std::accumulate(it, it, 0.0); +} + +int negative5() { + float a[1] = {0.5f}; + float b[1] = {1.0f}; + return std::inner_product(std::par, a, a + 1, b, 0.0f); +} + +namespace blah { +namespace std { +template +T accumulate(InputIt, InputIt, T); // We should not care about this one. +} + +int negative5() { + float a[1] = {0.5f}; + // Note that this is using blah::std::accumulate. + return std::accumulate(a, a + 1, 0); +} +}