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
