[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
Jonas Toth via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 24 03:12:21 PDT 2018
JonasToth added inline comments.
================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:58
+ const QualType Rhs) {
+ assert(Lhs->isRealType()); // Either integer or floating point.
+ assert(Rhs->isFloatingType()); // Floating point only.
----------------
gchatelet wrote:
> JonasToth wrote:
> > Couldn't be the conversion from an `int` to an `enum` be considered narrowing as well? (Not sure about the word of the standard) I think it makes sense to change the `assert` to `return false`
> This is a good point but I'd like to implement this as a second patch if you don't mind.
> I created this bug to track it:
> https://bugs.llvm.org/show_bug.cgi?id=39401
Implementing this in a follow-up is no problem, but could such a case trigger the assertion by any means?
================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:83
+ } else {
+ llvm_unreachable("Invalid state");
}
----------------
gchatelet wrote:
> hokein wrote:
> > We expect that there are only two possible cases here, how about rearrange the code like below, which I think it would simpler and improve code readability.
> >
> > ```
> > if (const auto *Op = Nodes.getNodeAs<BinaryOperator>("op")) {
> > // handle case for "op".
> > return;
> > }
> > const auto *Cast = Nodes.getNodeAs<ImplicitCastExpr>("cast");
> > assert(Cast && "must be cast cases");
> > // ...
> > ```
> I see.
> However I plan to add two improvement to this clang tidy so the number of cases will increase.
> What do you think about the following code?
> ```
> if (const auto *Op = Nodes.getNodeAs<BinaryOperator>("op"))
> return checkBinaryOperator(*Op);
> if (const auto *Cast = Nodes.getNodeAs<ImplicitCastExpr>("cast"))
> return checkCastOperator(*Cast);
> llvm_unreachable("must be binary operator or cast expression");
>
> ```
That works as well. The most important thing is that the intended code paths are explicitly visible and early return is preferred in llvm (and else after return forbidden :))
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53488
More information about the cfe-commits
mailing list