[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

Tue Apr 23 10:05:15 PDT 2019

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