[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Aaron Ballman via Phabricator via cfe-commits]

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