ztamas marked 2 inline comments as done. ztamas added inline comments.
================ Comment at: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351 + int *p; +}; ---------------- aaron.ballman wrote: > ztamas wrote: > > JonasToth wrote: > > > Please add tests with templated classes and self-assignment. > > I tested with template classes, but TemplateCopyAndSwap test case, for > > example, was wrongly caught by the check. So I just changed the code to > > ignore template classes. I did not find any template class related catch > > either in LibreOffice or LLVM when I run the first version of this check, > > so we don't lose too much with ignoring template classes, I think. > I am not keen on ignoring template classes for the check; that seems like a > bug in the check that should be addressed. At a minimum, the test cases > should be present with FIXME comments about the unwanted diagnostics. I don't think it's a good idea to change the check now to catch template classes and produce false positives. First of all the check does not work with template classes because the AST is different. Check TemplateCopyAndSwap test case for example. It's expected that the definition of operator= contains a CXXConstructExpr, but something is different for templates. It might be a lower level problem, how to detect a constructor call in a template class or templates just need different matcher. So implementing this check with correct template handling might be tricky and it might make the checker more complex. I'm not sure it worth the time, because as I mentioned I did not see any template related catch in the tested code bases. However, it might be a good idea to mention this limitation in the documentation. About the false positives. This template related false positives are different from other false positives. In general, check doesn't warn, if the code uses one of the three patterns (self-check, copy-and-move, copy-and-swap). However, TemplateCopyAndSwap test case was wrongly caught by the check even though it uses copy-and-swap method. I would not introduce this kind of false positives. So the user of the check can expect that if he / she uses one of the three patterns, then there will be no warning in his / her code. I already added five template related test cases. I think the comments are clear about which test case should be ignored by the check and which test cases are incorrectly ignored by now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new/ https://reviews.llvm.org/D60507 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
