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

This revision was automatically updated to reflect the committed changes. Closed by commit rL333066: [clang-tidy] new cppcoreguidelines-narrowing-conversions check. (authored by courbet, committed by ). Repository: rL LLVM https://reviews.llvm.org/D38455 Files:

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

courbet added a comment. Great, thanks for the review ! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. I think this generally LGTM. You should wait a bit to see if @alexfh has any other concerns. Comment at:

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

courbet added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35 + hasSourceExpression(IsFloatExpr), + unless(hasParent(castExpr( +

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

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35 + hasSourceExpression(IsFloatExpr), + unless(hasParent(castExpr( +

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

courbet added a comment. Thanks. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35 + hasSourceExpression(IsFloatExpr), + unless(hasParent(castExpr( +

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

courbet updated this revision to Diff 147508. courbet marked 2 inline comments as done. courbet added a comment. - More explicit documentation. - Do not trigger in template and macro contexts. - More tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 Files:

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

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35 + hasSourceExpression(IsFloatExpr), + unless(hasParent(castExpr( +

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

courbet added a comment. @aaron.ballman Ping Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

alexfh added a comment. In https://reviews.llvm.org/D38455#1062722, @courbet wrote: > In https://reviews.llvm.org/D38455#1062718, @JonasToth wrote: > > > > Sure. Is it OK to add a dependency from > > > clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in > > > clang-tidy/cpp-coreguidelines ?

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

courbet added a comment. In https://reviews.llvm.org/D38455#1062718, @JonasToth wrote: > > Sure. Is it OK to add a dependency from > > clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in > > clang-tidy/cpp-coreguidelines ? Is there an existing example of this ? > > Take a look in the hicpp

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

courbet updated this revision to Diff 141801. courbet added a comment. - Add NarrowingConversionsCheck to bugprone module. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt

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

JonasToth added a comment. > Sure. Is it OK to add a dependency from > clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in > clang-tidy/cpp-coreguidelines ? Is there an existing example of this ? Take a look in the hicpp module, almost everything there is aliased :) Repository: rCTE

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

courbet updated this revision to Diff 141800. courbet added a comment. - Simplify matcher expression - Add other binary operators - Add more tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt

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

courbet added a comment. In https://reviews.llvm.org/D38455#1061926, @pfultz2 wrote: > This seems like it would be nice to have in `bugprone-*` as well. Sure. Is it OK to add a dependency from `clang-tidy/bugprone/BugproneTidyModule.cpp` to stuff in `clang-tidy/cpp-coreguidelines` ? Is there

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

courbet added a comment. In https://reviews.llvm.org/D38455#1061681, @JonasToth wrote: > Could you please add some tests that include user defined literals and there > interaction with other literals. We should catch narrowing conversions from > them, too. User defined literals do not have

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

pfultz2 added a comment. This seems like it would be nice to have in `bugprone-*` as well. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

courbet added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:41 + binaryOperator( + anyOf(hasOperatorName("+="), hasOperatorName("-=")), + hasLHS(hasType(isInteger())), alexfh wrote: > JonasToth

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

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.

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

JonasToth added a comment. Could you please add some tests that include user defined literals and there interaction with other literals. We should catch narrowing conversions from them, too. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455

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

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:34 + hasSourceExpression(IsFloatExpr), + unless(hasParent(castExpr(.bind("cast"), + this); courbet wrote:

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

courbet updated this revision to Diff 141635. courbet marked 2 inline comments as done. courbet added a comment. - Add `*=` and `/=`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt

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

courbet marked 2 inline comments as done. courbet added a comment. Thanks. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:32 + Finder->addMatcher( + implicitCastExpr(hasImplicitDestinationType(isInteger()), +

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

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:32 + Finder->addMatcher( + implicitCastExpr(hasImplicitDestinationType(isInteger()), + hasSourceExpression(IsFloatExpr), Do you

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

courbet added a comment. In https://reviews.llvm.org/D38455#1061406, @JonasToth wrote: > > I think all comments must be marked as done to be ready for review > again. > > I think alexfh did disable the notifications for now. Add/ping him again. I see, thanks. Repository: rCTE Clang

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

JonasToth added a comment. > Is there anything I need to do for the diff to change state ? I thought updating the code would automatically mark it "ready for review" again. I think all comments must be marked as done to be ready for review again. Usually the reviewer reacts to changed code,

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

courbet added a comment. In https://reviews.llvm.org/D38455#1061300, @JonasToth wrote: > That sounds good. > > > Removing from my dashboard for now. > > maybe you should add alexfh again and discuss the results. Is there anything I need to do for the diff to change state ? I thought updating

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

courbet updated this revision to Diff 141610. courbet edited the summary of this revision. courbet added a comment. - Add support for bad cast detection. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt

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

JonasToth added a comment. That sounds good. > Removing from my dashboard for now. @aaron.ballman seems to be busy now, maybe you should add alexfh again and discuss the results. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455

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

courbet added a comment. Hi Jonas, In https://reviews.llvm.org/D38455#1061228, @JonasToth wrote: > Hi, > > my 2 cents: > > - On which codebases did you run the check? A large repository of open-source code, plus internal code at google. External code includes e.g. code from ffmpeg, Eigen, R,

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

JonasToth added a comment. Hi, my 2 cents: - On which codebases did you run the check? - did you consider looking for `implicitCastExpr`? You can capture all narrowing conversion with that and analyze them further. I think it is possible to warn for the subset mentioned in the guidelines. -

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

courbet added a comment. ping Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

courbet added a comment. OK, here's an analysis of 100 instances of a check that would warn on all fully implicit (no C/static/reinterpret/...) casts from float/double to integral: link I don't

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

alexfh requested changes to this revision. alexfh added a comment. This revision now requires changes to proceed. > I agree that more data would be useful, so I'll do an analysis of flagging > all (non-ceil/floor float/double)->integral conversions. Removing from my dashboard for now.

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

courbet added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:26 + binaryOperator( + anyOf(hasOperatorName("+="), hasOperatorName("-=")), + hasLHS(hasType(isInteger())), aaron.ballman wrote: >

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

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:26 + binaryOperator( + anyOf(hasOperatorName("+="), hasOperatorName("-=")), + hasLHS(hasType(isInteger())), courbet wrote: >

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

courbet added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:26 + binaryOperator( + anyOf(hasOperatorName("+="), hasOperatorName("-=")), + hasLHS(hasType(isInteger())), aaron.ballman wrote: > Why

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

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:26 + binaryOperator( + anyOf(hasOperatorName("+="), hasOperatorName("-=")), + hasLHS(hasType(isInteger())), Why only these two

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

courbet updated this revision to Diff 138910. courbet added a comment. Herald added a subscriber: cfe-commits. Do not trigger on `some_int += std::floor(some_float)`; Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt