ztamas marked 3 inline comments as done.
ztamas added inline comments.
================
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())))))));
----------------
aaron.ballman wrote:
> 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.
> I don't get that from my reading of the CERT rule, where it says [...]
Ah, Ok. I missed that part, so copy-and-move won't be a problem.
Now, I removed the cert alias, but later it can be added back when the check
can be configured so it conforms with the cert, I think.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:82
+ diag(MatchedDecl->getLocation(),
+ "operator=() might not handle self-assignment properly");
+}
----------------
aaron.ballman wrote:
> 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.
We don't actually know. We check only some patterns, but there might be other
operator=() implementations which are self assignment safe (see false positive
test cases).
================
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:
> > > > 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.
OK, I'll give it another try. Maybe it's better to not searching for a copy
constructor, but a variable declaration with the corresponding type.
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
