gchatelet marked an inline comment as done. gchatelet added inline comments.
================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:42 + unless(hasParent(castExpr())), + unless(isInTemplateInstantiation())) + .bind("cast"), ---------------- JonasToth wrote: > Here and in the matcher below: > > `isInTemplateInstantiation` is a wide range, if you match only on > `isTypeDependent()` it would remove the false negatives from templates but > still catch clear cases that are within a template function. > > With the current implementation non-type-templates would be ignored as well. > IMHO that could be done in a follow-up to keep the scope on one thing. Sounds good, let's fix this in a follow-up patch. ================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:92 + + // 'One' is created with PrecisionBits plus two bytes: + // - One to express the missing negative value of 2's complement ---------------- JonasToth wrote: > Maybe repleace `One` with `1.0`? It sounds slightly weird with the following > `One`s It's a leftover, I've reworded and changed the code as well. ================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:149 + const QualType RhsType = Rhs->getType(); + if (Lhs->isValueDependent() || Rhs->isValueDependent() || + LhsType->isDependentType() || RhsType->isDependentType()) ---------------- JonasToth wrote: > you can shorten this condition with `isDependentType` I tried using the following but it crashes when I run clang tidy other our codebase: ``` if (LhsType->isDependentType() || RhsType->isDependentType()) return false; ``` Maybe I misunderstood what you suggested? ================ Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:165 + if (const auto *CO = llvm::dyn_cast<ConditionalOperator>(Rhs)) { + const bool NarrowLhs = diagIfNarrowConstant( + Context, CO->getLHS()->getExprLoc(), Lhs, CO->getLHS()); ---------------- JonasToth wrote: > you could remove both temporaries and return directly and then ellide the > braces. > If you don't like that please remove the `const` I have to keep both variables or the first one might shortcircuit the second that goes undetected. e.g. `unsigned char c = b ? 1 : -1;` 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