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

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

Thank you for bearing with me  @aaron.ballman.



================
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.
+}
----------------
aaron.ballman wrote:
> Why is this function needed at all?
The two empty functions are placeholders so the cases handled by `ImplicitCast` and `BinaryOp` are the same.
I'll add some documentation.


================
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.
+}
----------------
aaron.ballman wrote:
> Why is this function needed at all?
Ditto.


================
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
----------------
aaron.ballman wrote:
> What variables are created here?
Leftover. I rewrote the comment.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53488/new/

https://reviews.llvm.org/D53488