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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
