aaron.ballman added inline comments.
================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:31 + Options.get("WarnOnFloatingPointNarrowingConversion", 1)), + WarnOnCastingLiterals(Options.get("WarnOnCastingLiterals", 1)) {} + ---------------- I think the concept here is more broad than the option name suggests -- this is really a "pedantic mode" for this feature, because there is a notional narrowing but no semantic difference. I would name the option `PedanticMode` and have it off-by-default, personally. ================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:127 + // symmetric and have the same number of positive and negative values. + // The range of valid integer for a floating point value is: + // [-2^PrecisionBits, 2^PrecisionBits] ---------------- integer -> integers ================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:242-244 + // "The resulting value is the smallest unsigned value equal to the source + // value modulo 2^n where n is the number of bits used to represent the + // destination type." ---------------- You should cite where this quotation comes from (e.g., [foo.bar]p2) ================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:259-263 +void NarrowingConversionsCheck::handleIntegralToBoolean( + const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs, + const Expr &Rhs) { + // Conversion from Integral to Bool value is well defined. +} ---------------- Why is this function needed at all? ================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:311-315 +void NarrowingConversionsCheck::handleBooleanToSignedIntegral( + const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs, + const Expr &Rhs) { + // Conversion from Bool to SignedIntegral value is well defined. +} ---------------- Why is this function needed at all? ================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:325 + if (Constant.isFloat()) { + // From http://eel.is/c++draft/dcl.init.list#7.2 + // Floating point constant narrowing only takes place when the value is ---------------- [dcl.init.list]p7.2 instead of a hyperlink. ================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:372 + if (const auto *CO = llvm::dyn_cast<ConditionalOperator>(&Rhs)) { + // We create variables so we make sure both sides of the + // ConditionalOperator are evaluated, the || operator would short ciruit ---------------- What variables are created here? ================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:373 + // We create variables so we make sure both sides of the + // ConditionalOperator are evaluated, the || operator would short ciruit + // the second call otherwise. ---------------- short ciruit -> short-circuit ================ Comment at: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst:59 +You may have encountered messages like so "narrowing conversion from +'unsigned int' to signed type 'int' is implementation-defined". ---------------- drop the "so" ================ Comment at: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst:61 +'unsigned int' to signed type 'int' is implementation-defined". +The C/C++ standard do not mandate two’s complement for signed integers, as +such the compiler is free to define what is the semantic for converting an ---------------- do not -> does not as such -> and so ================ Comment at: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst:62 +The C/C++ standard do not mandate two’s complement for signed integers, as +such the compiler is free to define what is the semantic for converting an +unsigned integer to signed integer. Clang's implementation uses the two’s ---------------- what is the semantic -> what the semantics are Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53488/new/ https://reviews.llvm.org/D53488 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits