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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
