[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet added a comment. Is this good enough to go? @JonasToth 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

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:178 + return; +// Conversions to unsigned integer are well defined and follow modulo 2 +// arithmetic. JonasToth wrote: > I am surprised by `follo

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet updated this revision to Diff 172904. gchatelet marked 2 inline comments as done. gchatelet added a comment. - Addressing comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 Files: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp clang-tidy/cpp

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added a comment. I think the check is really close. If the other reviewers want to take a look at it again, now is a good moment :) Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:178 + return; +// Conversions to unsigned integer are we

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet updated this revision to Diff 172778. gchatelet marked 7 inline comments as done. gchatelet added a comment. - Addressing comments and enhancing diagnostics Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 Files: clang-tidy/cppcoreguidelines/NarrowingConversions

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:164 + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + }

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet marked 8 inline comments as done. gchatelet added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:164 + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:164 + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + }

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

lebedev.ri added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:164 + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + } ---

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

courbet added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:155 + if (!LhsIntegerRange.Contains(IntegerConstant)) +diag(SourceLoc, "narrowing conversion from %0 to %1") << RhsType << LhsType; + return true; I think

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:149 + const QualType RhsType = Rhs->getType(); + if (Lhs->isValueDependent() || Rhs->isValueDependent() || + LhsType->isDependentType() || RhsType->isDependentType()) --

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet marked an inline comment as done. gchatelet added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:42 + unless(hasParent(castExpr())), + unless(isInTemplateInstantiation())) + .b

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet updated this revision to Diff 171826. gchatelet marked 12 inline comments as done. gchatelet added a comment. - Address comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 Files: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp clang-tidy/cppco

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet added a comment. Here are 15 random ones from **llvm** synced at `8f9fb8bab2e9b5b27fe40d700d2abe967b99fbb5`. lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3802:31: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] tools/dsymutil/Mach

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:25 // FIXME: Check double -> float truncation. Pay attention to casts: void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) { I think this

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet updated this revision to Diff 171717. gchatelet marked 4 inline comments as done. gchatelet added a comment. - Remove leftover Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 Files: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp clang-tidy/cppcore

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

Szelethus added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:14 #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/APInt.h" +#include "llvm/ADT/APSInt.h" Is `APInt` used anywhere? Repository: rCTE Cl

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added inline comments. Comment at: docs/ReleaseNotes.rst:77 - New :doc:`abseil-duration-factory-float ` check. JonasToth wrote: > that seems to be unrelated sry for noise, this was part of the history diffs! Repository: rCTE Clang Tools Extra

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth requested changes to this revision. JonasToth added a comment. This revision now requires changes to proceed. because this now diverged quite a bit i will revert the lgtm for now and take a longer look at the new patch. The numbers you reported sound good. Comment at

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet added a comment. So I ran `llvm/tools/clang/tools/extra/clang-tidy/tool/run-clang-tidy.py` The bare version produces `65664` unique findings. The version restricted to `cppcoreguidelines-narrowing-conversions` produces `4323` unique findings. That's about `+7%` of findings. I can post s

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet added a comment. So I've updated the code to add more narrowing conversions. It now tests constant expressions against receiving type, do not warn about implicit bool casting, handles ternary operator with constant expressions. I ran it on our code base: results look legit. I'll ping b

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet updated this revision to Diff 171692. gchatelet added a comment. - Add more checks, disable bool implicit casts warning, enable ternary operator warnings. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 Files: clang-tidy/cppcoreguidelines/NarrowingConversionsC

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added a comment. That one looks interesting :) Am 25.10.18 um 17:13 schrieb Guillaume Chatelet via Phabricator: > gchatelet added a comment. > > In https://reviews.llvm.org/D53488#1275786, @hokein wrote: > >> In https://reviews.llvm.org/D53488#1275750, @gchatelet wrote: >> >>> In ht

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet added a comment. In https://reviews.llvm.org/D53488#1275786, @hokein wrote: > In https://reviews.llvm.org/D53488#1275750, @gchatelet wrote: > > > In https://reviews.llvm.org/D53488#1274205, @JonasToth wrote: > > > > > Did you run this code over a real-world code-base and did you find ne

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

hokein added a comment. In https://reviews.llvm.org/D53488#1275750, @gchatelet wrote: > In https://reviews.llvm.org/D53488#1274205, @JonasToth wrote: > > > Did you run this code over a real-world code-base and did you find new > > stuff and/or false positives or the like? > > > Yes I did run it

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet marked 2 inline comments as done. gchatelet added a comment. In https://reviews.llvm.org/D53488#1274205, @JonasToth wrote: > Did you run this code over a real-world code-base and did you find new stuff > and/or false positives or the like? Yes I did run it over our code base. I didn'

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:56-57 -void NarrowingConversionsCheck::check(const MatchFinder::MatchResult &Result) { - if (const auto *Op = Result.Nodes.getNodeAs("op")) { -if (Op->getBeginLoc().

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. Did you run this code over a real-world code-base and did you find new stuff and/or false positives or the like? From my side LGTM unless other reviewers see outstanding issues. Reposi

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet updated this revision to Diff 170878. gchatelet marked an inline comment as done. gchatelet added a comment. - Join the two parts of the ReleaseNotes update Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 Files: clang-tidy/cppcoreguidelines/NarrowingConversions

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added inline comments. Comment at: docs/ReleaseNotes.rst:166 +- The :doc:`cppcoreguidelines-narrowing-conversions +` check. please concat the two parts, similar to the one above. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D5

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet added a comment. In https://reviews.llvm.org/D53488#1273986, @JonasToth wrote: > Please add a short notice in the release notes for this change. Sorry I keep on missing to update doc/release notes. Do you see anything else to add to the Patch? Repository: rCTE Clang Tools Extra h

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet updated this revision to Diff 170851. gchatelet added a comment. - Add documentation to ReleaseNotes Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 Files: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp clang-tidy/cppcoreguidelines/NarrowingConver

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added a comment. Please add a short notice in the release notes for this change. 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/ma

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

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

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet updated this revision to Diff 170846. gchatelet added a comment. - Update documentation, makes returning code path more explicit. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 Files: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp clang-tidy/cppc

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

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

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

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

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

hokein added a comment. Could you also update the check documentation `clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst`? Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:83 + } else { +llvm_unreachable("Invalid state"); } ---

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet updated this revision to Diff 170605. gchatelet marked 2 inline comments as done. gchatelet added a comment. - Addressing comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 Files: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp test/clang-tid

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

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

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added a comment. Please add a short sentence to the release notes and adjust the documentation to that check to cover the changes. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-comm

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

gchatelet created this revision. gchatelet added a reviewer: hokein. Herald added subscribers: cfe-commits, kbarton, xazax.hun, nemanjai. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 Files: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp test/clang-tidy/cpp