aaron.ballman added a comment. In D46317#2027330 <https://reviews.llvm.org/D46317#2027330>, @khuttun wrote:
> In D46317#2027071 <https://reviews.llvm.org/D46317#2027071>, @aaron.ballman > wrote: > > > In D46317#2023406 <https://reviews.llvm.org/D46317#2023406>, @khuttun wrote: > > > > > Any comments on this? Is this checker something that could be part of > > > clang-tidy? > > > > > > Thank you for posting some of the diagnostics found by the check, that was > > really helpful information! I spot-checked ~10 of the issues it reported > > and all of them were false positives. Were you able to find any true > > positives from that list? I think 1200 reports without any true positives > > indicates that the check may be too chatty to include (it may also suggest > > that `bugprone` is the wrong place for the check). > > > It's difficult to spot actual functionality bugs without knowing the code > better, but there's plenty of unnecessary double-lookups (count()/find() + > operator[]) reported, for example: Definitely agreed that it's labor-intensive. :-) > clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp:75:30 > > clang-tools-extra/clang-tidy/bugprone/ForwardDeclarationNamespaceCheck.cpp:157:33 > clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:106:29 I hadn't spot checked those, but did see at least one similar case with what I checked. However, I consider those false positives because none of these are cases where the code gives wrong results. It is possible they are performance concerns (though I'd be curious whether these patterns optimize well or not), so if the check was specific to finding that scenario, that would be pretty useful (though it likely requires codeflow analysis). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D46317/new/ https://reviews.llvm.org/D46317 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
