aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:54
+ recordType(hasDeclaration(classTemplateSpecializationDecl(
+ hasAnyName("std::shared_ptr", "std::unique_ptr", "std::weak_ptr",
+ "std::auto_ptr"),
----------------
ztamas wrote:
> aaron.ballman wrote:
> > These should be using `::std::whatever` to ensure we only care about names
> > in the global namespace named `std`.
> >
> > Should this list be configurable? For instance, boost has a fair number of
> > smart pointers.
> > Should this list be configurable? For instance, boost has a fair number of
> > smart pointers.
>
> xazax.hun has the same suggestion. I think it's a good idea for a follow-up
> patch. I actually added a test case with the name CustomPtrField to document
> this future possibility.
Yeah, I think it's fine to leave the list until a follow-up patch (though the
names should still be corrected for this patch).
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:62-64
+ const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(
+ has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
+ hasType(arrayType())))))));
----------------
ztamas wrote:
> aaron.ballman wrote:
> > Hmm, while I understand why you're doing this, I'm worried that it will
> > miss some pretty important cases. For instance, improper thread locking
> > could result in deadlocks, improper releasing of non-memory resources could
> > be problematic (such as network connections, file streams, etc), even
> > simple integer assignments could be problematic in theory:
> > ```
> > Yobble& Yobble::operator=(const Yobble &Y) {
> > superSecretHashVal = 0; // Being secure!
> > ... some code that may early return ...
> > superSecretHashVal = Y.superSecretHashVal;
> > }
> > ```
> > I wonder whether we want an option here to allow users to diagnose
> > regardless of whether we think it's suspicious or not.
> >
> > At the very least, this code should not be enabled for the CERT version of
> > the check as it doesn't conform to the CERT requirements.
> It's starting to be too much for me.
> First, the problem was false positives. If there are too many false positives
> then better to have it in the cert module.
> Now your problem is that this is not fit with the CERT requirements, because
> of this matcher which reduces the false positives. Adding this check to the
> CERT module was not my idea in the first place. So I suggest to have it a
> simple bugprone check (and remove the cert alias) and also we can mention
> that it is related to the corresponding cert rule (but it does not conform
> with it entirely).
> This check allows the usage of copy-and-move pattern, which does not conform
> with the cert specification either where only the self-check and
> copy-and-swap is mentioned. So your next suggestion will be to not allow
> copy-and-move because it does not fit with the cert rule? So I think it's
> better to have this not a cert check then, but a bugprone check. I prefer to
> have a working check then implementing a theoretical documentation.
>
> Apart from that cert thing, it actually seems a good idea to add a config
> option to allow the user to get all catches, not just the "suspicious ones".
> It's starting to be too much for me.
It can be tricky to get this stuff right; I'm sorry the difficulties are
aggravating.
> First, the problem was false positives. If there are too many false positives
> then better to have it in the cert module.
> Now your problem is that this is not fit with the CERT requirements, because
> of this matcher which reduces the false positives.
> Adding this check to the CERT module was not my idea in the first place. So I
> suggest to have it a simple bugprone check (and remove the cert alias) and
> also we can mention that it is related to the corresponding cert rule (but it
> does not conform with it entirely).
We typically ask authors to support the various coding standard modules when
plausible because of the considerable amount of overlap between the
functionalities. I don't particularly like the idea of ignoring the CERT coding
standard here given that the solution is almost trivial to implement. However,
if you want to remove it from the CERT module and not support it, that's your
choice.
> This check allows the usage of copy-and-move pattern, which does not conform
> with the cert specification either where only the self-check and
> copy-and-swap is mentioned.
I don't get that from my reading of the CERT rule, where it says, "A
user-provided copy assignment operator must prevent self-copy assignment from
leaving the object in an indeterminate state. This can be accomplished by
self-assignment tests, copy-and-swap, or other idiomatic design patterns.".
Copy-and-move is another idiomatic design pattern for dealing with this, and
I'm glad your check incorporates it. (tbh, it would be nice for the CERT rule
to have a compliant solution demonstrating it -- I'll recommend it on their
rule.)
> So your next suggestion will be to not allow copy-and-move because it does
> not fit with the cert rule? So I think it's better to have this not a cert
> check then, but a bugprone check. I prefer to have a working check then
> implementing a theoretical documentation.
Theoretical documentation? The CERT standard is a published standard used in
industry that's supported by other analyzers as well as clang-tidy, so it's not
really theoretical.
> Apart from that cert thing, it actually seems a good idea to add a config
> option to allow the user to get all catches, not just the "suspicious ones".
Agreed. FWIW, having a config option is what would make it trivial to support
in both modules. See `getModuleOptions()` in `CERTTidyModule.cpp` for an
example of how we control the behavior of
`readability-uppercase-literal-suffix` differently when its enabled through the
CERT check name.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:82
+ diag(MatchedDecl->getLocation(),
+ "operator=() might not handle self-assignment properly");
+}
----------------
ztamas wrote:
> aaron.ballman wrote:
> > Hmm, the "might not" seems a bit flat to me. How about: `'operator=()' does
> > not properly test for self-assignment`?
> >
> > Also, do we want to have a fix-it to insert a check for self assignment at
> > the start of the function?
> I don't think "test for self-assignment" will be good, because it's only one
> way to make the operator self assignment safe. In case of copy-and-swap and
> copy-and-move there is no testing of self assignment, but swaping/moving
> works in all cases without explicit self check.
>
> A fix-it can be a good idea for a follow-up patch.
Good point on the use of "test" in the diagnostic. My concern is more with it
sounding like we don't actually know -- the "might" is what's bothering me. I
think if we switch it to "does not", it'd be a bit better.
================
Comment at:
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351
+ int *p;
+};
----------------
ztamas wrote:
> 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> &'
>
> ```
Hmm, I believe it's this bit (which is different than the non-template case,
but still shows the initialization of a local variable of the expected type):
```
| | | |-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> &'
```
but the fact that the `ParenListExpr` has a null type is surprising to me.
Curiously enough, you get better type information with an initializer list or
an assignment expression as the initializer.
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
