This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Tweak 'rule of 3/5' checks to allow defaulting a destructor outside the class.
ClosedPublic

Authored by royjacobson on Feb 12 2023, 11:30 AM.

Details

Summary

A somewhat common code-pattern is to default a destructor in the source file and not in the header.
For example, this is the way to use smart pointers with forward-declared classes:

c++

struct Impl;
struct A {
  ~A(); // Can't be defaulted in the header.

private:
  std::unique_ptr<Impl> impl;
};

To be able to use this check with this pattern, I modified the behavior with AllowSoleDefaultDtor
to not trigger on destructors if they aren't defined yet.
Since a declared destructor should still be defined somewhere in the program, this
won't miss bad classes, just diagnose on less translation units.

Diff Detail

Event Timeline

royjacobson created this revision.Feb 12 2023, 11:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2023, 11:30 AM
royjacobson requested review of this revision.Feb 12 2023, 11:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2023, 11:30 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ccotter added inline comments.
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
41

typo: destuctor->destructor

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
28–29

Please follos 80 characters limit.

41

Ditto.

Address CR comments.

royjacobson marked 3 inline comments as done.Feb 13 2023, 8:24 AM
royjacobson edited the summary of this revision. (Show Details)Feb 13 2023, 8:35 AM

Thing is that same issue may happen with all other members, what about copy/move constructors defaulted in .cpp (just to speed up compilation for classes with many members).
Best thing would simply to check if all definitions are available. In that case issues would be reported only for source file that corresponds to header file with defined class.
This would also reduce amount of generated warnings for a big project that include such header with class in many places.

Other good option would be to excluded classes defined in system headers, no point to check if some boost class or std::vector got proper constructors.

In D143851#4126453, @ClockMan wrote:

Thing is that same issue may happen with all other members, what about copy/move constructors defaulted in .cpp (just to speed up compilation for classes with many members).
Best thing would simply to check if all definitions are available. In that case issues would be reported only for source file that corresponds to header file with defined class.
This would also reduce amount of generated warnings for a big project that include such header with class in many places.

Other good option would be to excluded classes defined in system headers, no point to check if some boost class or std::vector got proper constructors.

Defaulting destructors is usually fine, but explicitly defaulting copy/move constructors implicitly deletes the other special member functions which this check should diagnose.
This is why this option was only available for destructors in the first place, I imagine.

About the second point - this sounds pretty general, doesn't clang-tidy already filter diagnostics in system headers? At least Clang does.

This is why this option was only available for destructors in the first place, I imagine.

The cppcoreguidelines are a bit tricky to work with. Some of the rules are too strict to reasonably apply them in practice. We have brought this up with the authors of the guidelines but they have rejected proposals for modification. That's why in clang-tidy we aim at having by default the strict behavior so the check follows the rules "as-is", while also adding options that allow users to slightly deviate from the rules, to be more pragmatic. Thus if we want some new behavior that deviates from the rule text as it's written we should preferably implement it as an option.

About the second point - this sounds pretty general, doesn't clang-tidy already filter diagnostics in system headers? At least Clang does.

Yes, clang-tidy will not issue warnings in system headers, so checks should not need to explicitly handle that.

Yes, clang-tidy will not issue warnings in system headers, so checks should not need to explicitly handle that.

That's not so simple. At work we use clang-tidy for few big projects. I had to explicitly add to some checks unless(isExpansionInSystemHeader()) to gain some performance improvement of clang-tidy. This check is one of them.
Problem is that some matchers or check methods in checks are heavy, and when you got lot of system headers (boost, STL, generated interfaces, message structs, ...) you may end up easily with hundreds of hidden issues from single translation unit.
For some checks explicit exclude of system headers gives ~50% performance improvement, that result in faster CI feedback.

Best would be to have something more generic like: if --system-headers is not used, then exclude code as early as possible, before even constructing diagnostic information.

That's not so simple. At work we use clang-tidy for few big projects. I had to explicitly add to some checks unless(isExpansionInSystemHeader()) to gain some performance improvement of clang-tidy. This check is one of them.
Problem is that some matchers or check methods in checks are heavy, and when you got lot of system headers (boost, STL, generated interfaces, message structs, ...) you may end up easily with hundreds of hidden issues from single translation unit.
For some checks explicit exclude of system headers gives ~50% performance improvement, that result in faster CI feedback.

Best would be to have something more generic like: if --system-headers is not used, then exclude code as early as possible, before even constructing diagnostic information.

Yes, I meant from a "diagnostic" point of view. I agree that performance-wise it's a lot of work that is discarded at the end and would be good to avoid as early as possible. This sounds like something all checks would benefit from, so finding a general solution to that problem would be good. I believe the best forum to discuss this would be an RFC in Discourse, make sure to tag @njames93 as Code Owner.

carlosgalvezp accepted this revision.Feb 26 2023, 12:16 AM

LGTM, thanks for the contribution! Do you have commit rights or would you like that we land it for you? If so, please provide name and email for attribution.

This revision is now accepted and ready to land.Feb 26 2023, 12:16 AM

LGTM, thanks for the contribution! Do you have commit rights or would you like that we land it for you? If so, please provide name and email for attribution.

I have commit rights, but this is my first clang-tidy contribution :) Thanks!