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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:130-131
         "bugprone-throw-keyword-missing");
+    CheckFactories.registerCheck<TooSmallLoopVariableCheck>(
+        "bugprone-too-small-loop-variable");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
----------------
This should be done in a separate patch, as it's unrelated to this patch.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:19
+
+void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
+  // We don't care about deleted, default or implicit operator implementations.
----------------
You should only register these matchers when in C++ mode.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:10
+
+This check corresponds to the CERT C++ Coding Standard rule
+`OOP54-CPP. Gracefully handle self-copy assignment
----------------
ztamas wrote:
> aaron.ballman wrote:
> > Eugene.Zelenko wrote:
> > > alexfh wrote:
> > > > JonasToth wrote:
> > > > > It is worth an alias into the cert module. Please add this with this revision already.
> > > > Please add a corresponding alias into the cert module.
> > > See also other aliased checks for documentation examples.
> > I kind of wonder whether we want it as an alias in the CERT module or just move it into the CERT module directly (and maybe provide an alias to bugprone, if we think the fp rate is reasonable enough).
> Now, I just added a cert alias. I can move this check to cert module entirely if needed. In general, I prefer a meaningful name over a cert number, that's why it might be more useful to have also a bugprone prefixed check for this. And it seems to me cert checks used to be the aliases.
If the purpose to the check is to conform to a specific coding standard, we usually put the check with the coding standard it supports unless the check is generally useful (then we put it into a module and alias into the coding standard module). I'm having a hard time convincing myself which way to go in this case, so I guess that means I don't have a strong opinion. :-)


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:6
+
+`cert-oop54-cpp` redirects here as an alias for this check.
+
----------------
You should add a link to CERT's documentation somewhere around this text.


================
Comment at: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351
+  int *p;
+};
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507