ztamas marked an inline comment 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: > > 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. > > So implementing this check with correct template handling might be tricky > > and it might make the checker more complex. > > I would be surprised if it added that much complexity. You wouldn't be > checking the template declarations, but the template instantiations > themselves, which should have the same AST representation as similar > non-templated code. > > > 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. > > It's needed to conform to the CERT coding standard, which has no exceptions > for templates here. > > > However, it might be a good idea to mention this limitation in the > > documentation. > > My preference is to support it from the start, but if we don't support it, it > should definitely be mentioned in the documentation. I added instatiation of template classes to the test code (locally): ``` template <class T> class TemplateCopyAndMove { public: TemplateCopyAndMove<T> &operator=(const TemplateCopyAndMove<T> &object) { TemplateCopyAndMove<T> temp(object); *this = std::move(temp); return *this; } private: T *p; }; int instaniateTemplateCopyAndMove() { TemplateCopyAndMove<int> x; (void) x; } ``` However I don't see the expected AST neither in the ClassTemplateDecl or in the ClassTemplateSpecializationDecl. So how can I get that AST which is similar to non-template case? ``` |-ClassTemplateDecl 0x117ed20 <line:341:1, line:352:1> line:342:7 TemplateCopyAndMove | |-TemplateTypeParmDecl 0x117ec08 <line:341:11, col:17> col:17 referenced class depth 0 index 0 T | |-CXXRecordDecl 0x117ec90 <line:342:1, line:352:1> line:342:7 class TemplateCopyAndMove definition | | |-DefinitionData standard_layout | | | |-DefaultConstructor exists trivial needs_implicit | | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param | | | |-MoveConstructor | | | |-CopyAssignment non_trivial has_const_param user_declared implicit_has_const_param | | | |-MoveAssignment | | | `-Destructor simple irrelevant trivial needs_implicit | | |-CXXRecordDecl 0x117ef70 <col:1, col:7> col:7 implicit class TemplateCopyAndMove | | |-AccessSpecDecl 0x117f000 <line:343:1, col:7> col:1 public | | |-CXXMethodDecl 0x117f9d0 <line:344:3, line:348:3> line:344:27 operator= 'TemplateCopyAndMove<T> &(const TemplateCopyAndMove<T> &)' | | | |-ParmVarDecl 0x117f820 <col:37, col:67> col:67 referenced object 'const TemplateCopyAndMove<T> &' | | | `-CompoundStmt 0x117fe30 <col:75, line:348:3> | | | |-DeclStmt 0x117fcb8 <line:345:5, col:40> | | | | `-VarDecl 0x117fc10 <col:5, col:39> col:28 referenced temp 'TemplateCopyAndMove<T>':'TemplateCopyAndMove<T>' callinit | | | | `-ParenListExpr 0x117fc98 <col:32, col:39> 'NULL TYPE' | | | | `-DeclRefExpr 0x117fbc0 <col:33> 'const TemplateCopyAndMove<T>':'const TemplateCopyAndMove<T>' lvalue ParmVar 0x117f820 'object' 'const TemplateCopyAndMove<T> &' | | | |-BinaryOperator 0x117fda8 <line:346:5, col:27> '<dependent type>' '=' | | | | |-UnaryOperator 0x117fce0 <col:5, col:6> '<dependent type>' prefix '*' cannot overflow | | | | | `-CXXThisExpr 0x117fcd0 <col:6> 'TemplateCopyAndMove<T> *' this | | | | `-CallExpr 0x117fd80 <col:13, col:27> '<dependent type>' | | | | |-UnresolvedLookupExpr 0x117fd18 <col:13, col:18> '<overloaded function type>' lvalue (no ADL) = 'move' 0x1137968 | | | | `-DeclRefExpr 0x117fd60 <col:23> 'TemplateCopyAndMove<T>':'TemplateCopyAndMove<T>' lvalue Var 0x117fc10 'temp' 'TemplateCopyAndMove<T>':'TemplateCopyAndMove<T>' | | | `-ReturnStmt 0x117fe20 <line:347:5, col:13> | | | `-UnaryOperator 0x117fe08 <col:12, col:13> '<dependent type>' prefix '*' cannot overflow | | | `-CXXThisExpr 0x117fdf8 <col:13> 'TemplateCopyAndMove<T> *' this | | |-AccessSpecDecl 0x117fa78 <line:350:1, col:8> col:1 private | | `-FieldDecl 0x117fad8 <line:351:3, col:6> col:6 p 'T *' | `-ClassTemplateSpecializationDecl 0x117ff38 <line:341:1, line:352:1> line:342:7 class TemplateCopyAndMove definition | |-DefinitionData pass_in_registers standard_layout literal | | |-DefaultConstructor exists trivial | | |-CopyConstructor simple trivial has_const_param implicit_has_const_param | | |-MoveConstructor | | |-CopyAssignment non_trivial has_const_param user_declared implicit_has_const_param | | |-MoveAssignment | | `-Destructor simple irrelevant trivial needs_implicit | |-TemplateArgument type 'int' | |-CXXRecordDecl 0x11801b0 prev 0x117ff38 <col:1, col:7> col:7 implicit class TemplateCopyAndMove | |-AccessSpecDecl 0x1180240 <line:343:1, col:7> col:1 public | |-CXXMethodDecl 0x1180550 <line:344:3, line:348:3> line:344:27 operator= 'TemplateCopyAndMove<int> &(const TemplateCopyAndMove<int> &)' | | `-ParmVarDecl 0x1180430 <col:37, col:67> col:67 object 'const TemplateCopyAndMove<int> &' | |-AccessSpecDecl 0x1180608 <line:350:1, col:8> col:1 private | |-FieldDecl 0x1180668 <line:351:3, col:6> col:6 p 'int *' | |-CXXConstructorDecl 0x11806e8 <line:342:7> col:7 implicit used TemplateCopyAndMove 'void () noexcept' inline default trivial | | `-CompoundStmt 0x1181528 <col:7> | `-CXXConstructorDecl 0x11813a0 <col:7> col:7 implicit constexpr TemplateCopyAndMove 'void (const TemplateCopyAndMove<int> &)' inline default trivial noexcept-unevaluated 0x11813a0 | `-ParmVarDecl 0x11814b8 <col:7> col:7 'const TemplateCopyAndMove<int> &' ``` 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