[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D38455#1061195, @courbet wrote:

> ping


Sorry for the long review due to the holidays.

Generally, I would also like Aaron to take a look when he's back, since he had some concerns. While we're waiting, one minor comment from me.



================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:41
+      binaryOperator(
+          anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+          hasLHS(hasType(isInteger())),
----------------
JonasToth wrote:
> courbet wrote:
> > JonasToth wrote:
> > > I would really like to add all other calc-and-assign operations. At least "*=" and "/=".
> > Makes sense, I've added `*=` and `/=`. All others (`%=`, `&=`, `<<=` and variations thereof)  trigger a compilation error if the RHS is a float (rightfully).
> > 
> > 
> For floats that is true.
> 
> If we care about the `ìnt->char` casts later, they should be added.
Does it make sense to match all assignment operators including just `=`? If so, the matcher will become a bit simpler: `binaryOperator(isAssignmentOperator(), ...)`. If there's a reason not to do this, maybe adding a comment would be useful

Apart from that, I wonder whether the matcher above would also cover the case of assignment operators? I would expect the AST for assignment expressions with different types to have ImplicitCast nodes anyway.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455