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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
