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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Only a few nits from me.



================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:30
+      WarnOnFloatingPointNarrowingConversion(
+          Options.get("WarnOnFloatingPointNarrowingConversion", 0)) {}
+
----------------
I would make this on by default. It is a valid diagnostic and people using this check might be surprised they have to activate it.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:169
+    // Integer to Bool type conversions are well defined and not considered as a
+    // narrowing. The following clang tidy already flags invalid boolean
+    // conversions.
----------------
s/clang tidy already/clang-tidy check already/ (not sure if clang-tidy-check is correcter from english point of view.)


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:206
+
+  assert(false && "Unhandled type conversion");
+}
----------------
i think `llvm_unreachable` would fit better.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:299
+      Rhs->isCXX11ConstantExpr(Context, &Constant)) {
+    if (Constant.isFloat())
+      diagIfNarrowFloatingPointConstant(Context, SourceLoc, Lhs, Rhs, Constant);
----------------
Why is the return here `true` even without diagnostic?


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:344
+                          Cast->getExprLoc(), Cast, Cast->getSubExpr());
+  assert(false && "must be binary operator or cast expression");
 }
----------------
`llvm_unreachable`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488