[PATCH] D26138: [clang-tidy] Add modernize-use-equals-delete check

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/modernize/UseEqualsDeleteCheck.cpp:29
+            cxxMethodDecl(
+                anyOf(isCopyAssignmentOperator(), isMoveAssignmentOperator())),
+            cxxDestructorDecl()));
----------------
malcolm.parsons wrote:
> aaron.ballman wrote:
> > malcolm.parsons wrote:
> > > aaron.ballman wrote:
> > > > How about a conversion operator, like `operator bool()`? You'll sometimes see that one declared privately for similar reasons.
> > > I haven't seen that. Do you have an example?
> > anecdote != data, and all that, but: http://stackoverflow.com/questions/5753460/a-way-to-disable-conversion-operators
> > 
> > I do agree though, this is not as common as noncopyable classes.
> I think I'll leave conversion operators to future modernize-use-explicit-conversion-operators or cppcoreguidelines-avoid-conversion-operators checks.
I'm okay with that.


================
Comment at: clang-tidy/modernize/UseEqualsDeleteCheck.cpp:52
+  diag(SpecialFunction->getLocation(),
+       "use '= delete' to prevent a default special member function")
+      << FixItHint::CreateInsertion(EndLoc, " = delete");
----------------
malcolm.parsons wrote:
> aaron.ballman wrote:
> > malcolm.parsons wrote:
> > > aaron.ballman wrote:
> > > > This diagnostic isn't very clear to me -- what does it mean to "prevent" a default special member function?
> > > > 
> > > > The fixit for this is also somewhat unsatisfying as this takes a private, not-defined function and turns it into a private, deleted function. That's a very small win, because it only impacts code which could access the special member function in the first place (some compilers give a diagnostic about the special member function being inaccessible even if it's explicitly marked as deleted; clang is not one such compiler). Do we have a way to rewrite the access specifier for the special member function as well (kind of like how we have a way to handle includes we're adding)? I am guessing not yet, but if we do, that would be fantastic to use here.
> > > > 
> > > > Note, I don't think this should hold up your patch or the fixit. A small win is still a win. :-)
> > > Do you have a better wording for the diagnostic?
> > > 
> > > I don't see any utility functions to make a method public.
> > Perhaps: "special member function with private access specifier and no definition is still accessible; use '= delete' to explicitly disallow all access"?
> > 
> > Or a less-wordy variant.
> How about "use '= delete' to prohibit calling of a special member function".
Given that this is in the modernize module, I think that's reasonable wording.


https://reviews.llvm.org/D26138