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