Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -6,6 +6,7 @@ AssignOperatorSignatureCheck.cpp BoolPointerImplicitConversionCheck.cpp DefinitionsInHeadersCheck.cpp + FoldInitTypeCheck.cpp ForwardDeclarationNamespaceCheck.cpp InaccurateEraseCheck.cpp IncorrectRoundings.cpp Index: clang-tidy/misc/FoldInitTypeCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/FoldInitTypeCheck.h @@ -0,0 +1,40 @@ +//===--- 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; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FOLD_INIT_TYPE_H Index: clang-tidy/misc/FoldInitTypeCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/FoldInitTypeCheck.cpp @@ -0,0 +1,126 @@ +//===--- 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 { + +/// Returns the value_type for an InputIterator type. +static QualType getInputIteratorValueType(const Type &IteratorType, + const ASTContext &context) { + if (IteratorType.isPointerType()) { + return IteratorType.getPointeeType().getCanonicalType(); + } + // Try to find the value_type. The actual condition is less restrictive + // (std::iterator_traits::value_type should exist). + if (IteratorType.isRecordType()) { + const CXXRecordDecl *IteratorDecl = + IteratorType.getAsCXXRecordDecl()->getCanonicalDecl(); + assert(IteratorDecl != nullptr); + for (const auto &InnerDecl : IteratorDecl->decls()) { + if (const auto *InnerTypeDecl = dyn_cast(InnerDecl)) { + if (InnerTypeDecl->getName() == "value_type") { + return context.getTypeDeclType(InnerTypeDecl).getCanonicalType(); + } + } + } + } + return QualType(); +} + +/// Returns true if ValueType is allwed to fold into InitType. +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; +} + +void FoldInitTypeCheck::registerMatchers(MatchFinder *Finder) { + // We match functions of interest and bind the iterator and init value types. + + // template< class InputIt, class T > + // T accumulate(InputIt first, InputIt last, T init); + Finder->addMatcher( + callExpr(callee(functionDecl( + hasName("std::accumulate"), + hasParameter(0, parmVarDecl().bind("iterator")), + hasParameter(2, parmVarDecl().bind("init_value")))), + argumentCountIs(3)) + .bind("call"), + this); +} + +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 *InitValue = Result.Nodes.getNodeAs("init_value"); + const auto *Iterator = Result.Nodes.getNodeAs("iterator"); + assert(InitValue != nullptr); + assert(Iterator != nullptr); + + QualType InitType = InitValue->getType(); + assert(!InitType.isNull()); + assert(!Iterator->getType().isNull()); + QualType ValueType = + getInputIteratorValueType(*Iterator->getType(), *Result.Context); + if (ValueType.isNull()) { + return; + } + + // Right now we check only builtin types. We expect people who use custom + // types to know what they are doing. + if (InitType->isBuiltinType() && ValueType->isBuiltinType()) { + const auto *InitBuiltinType = InitType->getAs(); + assert(InitBuiltinType != nullptr); + const auto *ValueBuiltingType = ValueType->getAs(); + assert(ValueBuiltingType != nullptr); + if (!isValidBuiltinFold(*ValueBuiltingType, *InitBuiltinType, + *Result.Context)) { + const PrintingPolicy Policy((LangOptions())); + diag(Result.Nodes.getNodeAs("call")->getExprLoc(), + "folding type '%0' into type '%1' might result in " + "loss of precision") + << ValueBuiltingType->getName(Policy) + << InitBuiltinType->getName(Policy); + } + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -15,6 +15,7 @@ #include "AssignOperatorSignatureCheck.h" #include "BoolPointerImplicitConversionCheck.h" #include "DefinitionsInHeadersCheck.h" +#include "FoldInitTypeCheck.h" #include "ForwardDeclarationNamespaceCheck.h" #include "InaccurateEraseCheck.h" #include "IncorrectRoundings.h" @@ -56,6 +57,8 @@ "misc-bool-pointer-implicit-conversion"); CheckFactories.registerCheck( "misc-definitions-in-headers"); + CheckFactories.registerCheck( + "misc-fold-init-type"); CheckFactories.registerCheck( "misc-forward-declaration-namespace"); CheckFactories.registerCheck( Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -51,6 +51,7 @@ misc-assign-operator-signature misc-bool-pointer-implicit-conversion misc-definitions-in-headers + misc-fold-init-type misc-forward-declaration-namespace misc-inaccurate-erase misc-incorrect-roundings Index: docs/clang-tidy/checks/misc-fold-init-type.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-fold-init-type.rst @@ -0,0 +1,25 @@ +.. 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 a 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: test/clang-tidy/misc-fold-init-type.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-fold-init-type.cpp @@ -0,0 +1,66 @@ +// RUN: %check_clang_tidy %s misc-fold-init-type %t + +namespace std { +template +T accumulate(InputIt first, InputIt last, T init) { + return T(); +} +} // namespace std + +template struct Iterator { typedef ValueType value_type; }; + +// Positives. + +int positive1() { + float a[1] = {0.5f}; + return std::accumulate(a, a + 1, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type + // 'int' might result in loss of precision [misc-fold-init-type] +} + +int positive2() { + Iterator it; + return std::accumulate(it, it, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type + // 'int' might result in loss of precision [misc-fold-init-type] +} + +int positive3() { + 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' might result in loss of precision [misc-fold-init-type] +} + +int positive4() { + Iterator it; + return std::accumulate(it, it, 0); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'unsigned int' into + // type 'int' might result in loss of precision [misc-fold-init-type] +} + +// 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() { + Iterator it; + // For now this is OK. + return std::accumulate(it, it, 0.0); +}