gchatelet 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.
----------------
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
================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:83
+ } else {
+ llvm_unreachable("Invalid state");
}
----------------
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");
```
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
