Page MenuHomePhabricator

[clang-tidy] Add modernize-use-equals-delete check
ClosedPublic

Authored by malcolm.parsons on Oct 31 2016, 3:52 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

malcolm.parsons retitled this revision from to [clang-tidy] Add modernize-use-delete check.
malcolm.parsons updated this object.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

Eugene.Zelenko added a subscriber: Prazek.

Add to release notes.

alexfh edited edge metadata.Oct 31 2016, 9:03 AM

"Use delete" makes me think of the delete operator. I'd suggest one of these names:

  • modernize-use-equals-delete
  • modernize-use-deleted-special-members
  • modernize-delete-special-members

(feel free to suggest a better alternative).

clang-tidy/modernize/UseDeleteCheck.cpp
37 ↗(On Diff #76374)

The double negation makes this part hard to parse. Can you add a comment saying (roughly) that the matcher ensures that all non-special functions declared in this class are defined in some way in the translation unit we're looking at?

"Use delete" makes me think of the delete operator. I'd suggest one of these names:

I chose modernize-use-delete as there's already modernize-use-default.
"Use default" makes me think of the default case in a switch.

  • modernize-use-equals-delete

Works for me, but should I rename modernize-use-default?

malcolm.parsons edited edge metadata.

Rename to modernize-use-equals-delete.
Add comment.

Sort headers and CMakeLists.txt.

malcolm.parsons retitled this revision from [clang-tidy] Add modernize-use-delete check to [clang-tidy] Add modernize-use-equals-delete check.Oct 31 2016, 10:41 AM

It'll be good idea to rename modernize-use-default in similar fashion, since both checks are closely relate and in both cases keywords are ambiguous.

aaron.ballman added inline comments.Oct 31 2016, 2:15 PM
clang-tidy/modernize/UseEqualsDeleteCheck.cpp
29 ↗(On Diff #76433)

How about a conversion operator, like operator bool()? You'll sometimes see that one declared privately for similar reasons.

37 ↗(On Diff #76433)

Missing a full stop at the end of the comment.

46 ↗(On Diff #76433)

Should rename this to not hide the global SpecialFunction object, and use the global SpecialFunction object in place of the string literal here. Alternatively, leave this name alone and remove the global variable and just use the string literal; either is fine.

52 ↗(On Diff #76433)

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. :-)

test/clang-tidy/modernize-use-equals-delete.cpp
93 ↗(On Diff #76433)

Phab will not let me delete the unsubmitted comment I had about this, so I am leaving a junk comment instead. Your move, Phabricator.

clang-tidy/modernize/UseEqualsDeleteCheck.cpp
29 ↗(On Diff #76433)

I haven't seen that. Do you have an example?

52 ↗(On Diff #76433)

Do you have a better wording for the diagnostic?

I don't see any utility functions to make a method public.

Fix comment.
Rename variable.
Use global.

malcolm.parsons marked 2 inline comments as done.Oct 31 2016, 4:06 PM

Rename modernize-use-default to modernize-use-equals-default

aaron.ballman added inline comments.Nov 1 2016, 6:55 AM
clang-tidy/modernize/UseEqualsDeleteCheck.cpp
29 ↗(On Diff #76433)

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.

52 ↗(On Diff #76433)

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.

malcolm.parsons added inline comments.Nov 1 2016, 7:45 AM
clang-tidy/modernize/UseEqualsDeleteCheck.cpp
29 ↗(On Diff #76433)

I think I'll leave conversion operators to future modernize-use-explicit-conversion-operators or cppcoreguidelines-avoid-conversion-operators checks.

52 ↗(On Diff #76433)

How about "use '= delete' to prohibit calling of a special member function".

aaron.ballman added inline comments.Nov 1 2016, 7:55 AM
clang-tidy/modernize/UseEqualsDeleteCheck.cpp
29 ↗(On Diff #76433)

I'm okay with that.

52 ↗(On Diff #76433)

Given that this is in the modernize module, I think that's reasonable wording.

Reword diagnostic.

Add FIXME comment.

Add quick hack to make methods public.
Cleanup around replacements should be enhanced with rules to remove
empty private/protected/public sections.
private: public: -> public:
public: private: -> private:

aaron.ballman edited edge metadata.Nov 2 2016, 9:30 AM

Can you split the modernize-use-default changes into a separate patch? I think that change requires additional discussion, because renaming the check may break users relying on the old name. The changes aren't really related (aside from naming consistency), and I'd rather not let modernize-use-default discussion delay accepting this patch.

clang-tidy/modernize/UseEqualsDeleteCheck.cpp
58 ↗(On Diff #76561)

I am on the fence about this fixit. On the one hand, the fix is a technical improvement because it means that implementations will consistently find the declaration and bark about it being explicitly deleted. On the other hand, the fixit suggests code that should never pass a reasonable code review.

I'm wondering if it would make more sense to leave the access specifiers alone and just put a FIXME in the code to point the situation out. I am guessing that at some point we will have a refactoring tool that can help without resorting to making declarations like public: C() = delete; private:.

What do you think?

malcolm.parsons edited edge metadata.

Take out renaming of modernize-use-default.

clang-tidy/modernize/UseEqualsDeleteCheck.cpp
58 ↗(On Diff #76561)

I'm wondering whether no fixit would be better than a not-good-enough fixit.

aaron.ballman added inline comments.Nov 2 2016, 10:16 AM
clang-tidy/modernize/UseEqualsDeleteCheck.cpp
58 ↗(On Diff #76561)

Generally, no. Incremental improvements are almost always fine. However, the user is asking to have their code modernized, and the fixit results in code that looks more foreign than modern (at least, to me).

I won't block the patch moving forward regardless of whether the fixit is in or out, but I am curious if @alexfh has an opinion, or if you have a strong preference one way or the other.

malcolm.parsons planned changes to this revision.Nov 5 2016, 4:49 AM
malcolm.parsons added inline comments.
clang-tidy/modernize/UseEqualsDeleteCheck.cpp
58 ↗(On Diff #76561)

I'll change the check to warn about deleted methods that aren't public; the user can fix those manually until a better fixit is possible.

Check for non-public deleted methods.
Remove imperfect FixItHint.
Add FIXME comments.

aaron.ballman accepted this revision.Nov 10 2016, 8:43 AM
aaron.ballman edited edge metadata.

LGTM, thank you!

clang-tidy/modernize/UseEqualsDeleteCheck.cpp
58 ↗(On Diff #76561)

I think that's a good solution for now, thanks!

This revision is now accepted and ready to land.Nov 10 2016, 8:43 AM
This revision was automatically updated to reflect the committed changes.