This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] New check cppcoreguidelines-prefer-member-initializer
ClosedPublic

Authored by baloghadamsoftware on Dec 9 2019, 5:03 AM.

Details

Summary

Finds member initializations in the constructor body which can be placed into the initialization list instead. This does not only improves the readability of the code but also affects positively its performance. Class-member assignments inside a control statement or following the first control statement are ignored.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2019, 5:03 AM

affects positively its performance.

Interesting! Do you have some numbers you could share? thanks

Can you refresh my memory on whether a rule for "if init expr is constant, initialise in class body instead" exists for init list members? If so, this will be a funny "two pass needed to fix" kind of check.

clang-tools-extra/clang-tidy/readability/PreferInitializationListCheck.cpp
19 ↗(On Diff #232816)

I'm working on a checker which has the need for similarly knowing occurrences of "control flow breaking statements". How about goto and calling a [[noreturn]] function, such as (std::)longjmp? Or there is no point in matching such in your checker?

71 ↗(On Diff #232816)

FIXME remained. Did you upload the right patch set?

85 ↗(On Diff #232816)

can -> should?

clang-tools-extra/clang-tidy/readability/PreferInitializationListCheck.h
18 ↗(On Diff #232816)

FIXME remained here.

clang-tools-extra/docs/clang-tidy/checks/readability-prefer-initialization-list.rst
7 ↗(On Diff #232816)

improves -> improve

8 ↗(On Diff #232816)

word order: also positively affects

27 ↗(On Diff #232816)

typo: construcotr

27–28 ↗(On Diff #232816)

[l]ist, unlike m, as m's initialization follow a control statement (if)

whisperity added inline comments.Dec 9 2019, 5:45 AM
clang-tools-extra/test/clang-tidy/checkers/readability-prefer-initialization-list.cpp
221 ↗(On Diff #232816)

Comment diverged from what's actually written in the code.

Clang-tidy also has modernize-use-default-member-init. Will be good idea to mention this check in documentation and otherwise as well as draw distinction (C++ version, coding guidelines, etc) in use cases.

clang-tools-extra/clang-tidy/readability/PreferInitializationListCheck.cpp
66 ↗(On Diff #232816)

Will be good idea to check for C++.

clang-tools-extra/docs/ReleaseNotes.rst
180

Please synchronize with first statement in documentation.

baloghadamsoftware retitled this revision from [clang-tidy] New check readability-prefer-initialization-list to [clang-tidy][WIP] New check readability-prefer-initialization-list.Dec 10 2019, 12:16 AM

Updated according to some comments.

baloghadamsoftware marked 7 inline comments as done.Dec 10 2019, 6:48 AM

Can you refresh my memory on whether a rule for "if init expr is constant, initialise in class body instead" exists for init list members? If so, this will be a funny "two pass needed to fix" kind of check.

This worries me as well -- we already have checks that prefer doing an in-class initialization to using a constructor member initialization list. How should this check interact with modernize-use-default-member-init?

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
91 ↗(On Diff #233087)

I have a concern with the name using initialization-list -- that sounds a lot like the check is going to prefer {42} to = 42. Perhaps the name should be prefer-ctor-member-initialization or something along those lines?

Can you refresh my memory on whether a rule for "if init expr is constant, initialise in class body instead" exists for init list members? If so, this will be a funny "two pass needed to fix" kind of check.

This worries me as well -- we already have checks that prefer doing an in-class initialization to using a constructor member initialization list. How should this check interact with modernize-use-default-member-init?

This check is to enforce C++ core guideline C.49 while the modernize check enforces guideline C.48. The two must be synchronized, and I think that this new check should do that: for initializations that should be done using in-class initializers according to the other checker this checker must suggest the same. For the rest we should suggest member initializers in the constructor.

This check is to enforce C++ core guideline C.49 while the modernize check enforces guideline C.48. The two must be synchronized, and I think that this new check should do that: for initializations that should be done using in-class initializers according to the other checker this checker must suggest the same. For the rest we should suggest member initializers in the constructor.

It should be in cppcoreguidelines module.

baloghadamsoftware retitled this revision from [clang-tidy][WIP] New check readability-prefer-initialization-list to [clang-tidy][WIP] New check cppcoreguidelines-prefer-initialization-list.

Now checker proposes default member initialization if applicable. Thus it is in sync with checker modernize-use-default-member-init. Also moved to ícppcoreguidelinesí group.

baloghadamsoftware retitled this revision from [clang-tidy][WIP] New check cppcoreguidelines-prefer-initialization-list to [clang-tidy] New check cppcoreguidelines-prefer-initialization-list.Jan 15 2020, 1:26 AM
Eugene.Zelenko added inline comments.Jan 15 2020, 3:15 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
53

Please elide braces.

clang-tools-extra/docs/ReleaseNotes.rst
70

Is it relevant?

73

Is it relevant?

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-member-initializer.rst
16

Enum should be in double back-ticks as language construct.

Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.

Updated according to the comments.

baloghadamsoftware marked 6 inline comments as done.Jan 15 2020, 4:17 AM

Thank you for the comments.

clang-tools-extra/docs/ReleaseNotes.rst
70

Absouletely not, it is nonsense. Just added by the renaming script without me knowing about it.

73

The same as above.

By the word, please rebase, because Release Notes were reset after version 10 branching.

baloghadamsoftware marked 2 inline comments as done.

Rebased.

Eugene.Zelenko added inline comments.Jan 16 2020, 6:03 AM
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-member-initializer.rst
73

Please make indentation 2 characters, same as in above code snippets.

baloghadamsoftware retitled this revision from [clang-tidy] New check cppcoreguidelines-prefer-initialization-list to [clang-tidy] New check cppcoreguidelines-prefer-initializer.

Indentation of the code fixed in the documentation.

baloghadamsoftware retitled this revision from [clang-tidy] New check cppcoreguidelines-prefer-initializer to [clang-tidy] New check cppcoreguidelines-prefer-member-initializer.Jan 16 2020, 9:31 AM
aaron.ballman added inline comments.Feb 1 2020, 8:04 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
48

Please keep this list sorted alphabetically.

clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
44

What about other kinds of literals like user-defined literals, or literal class types? Should this be using E->getType()->isLiteralType()?

baloghadamsoftware marked 2 inline comments as done.Feb 25 2020, 5:49 AM
baloghadamsoftware added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
44

E->getType()->isLiteralType() does not work since it tells about the type, not the expression itself. Here we should return true for literals only, not for any expressions whose type is a literal type. Thus int is a literal type, 5 is an int literal, but n or return_any_int() not.

aaron.ballman added inline comments.Feb 28 2020, 8:08 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
26

How about throw expressions?

62–66
if (const auto *DRE = dyn_cast<DeclRefExpr>(Value))
  return isa<EnumConstantDecl>(DRE->getDecl())
return false;
127–129

This can be hoisted into the matcher with hasBody(anything()) I believe. One interesting test case that's somewhat related are constructors that use a function-try-block instead of a compound statement. e.g.,

class C {
  int i;

public:
  C() try {
    i = 12;
  } catch (...) {
  }
};
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-member-initializer.rst
7

placed to the -> converted into

8

does not -> not

Updated according to the comments.

baloghadamsoftware marked 7 inline comments as done.May 26 2020, 1:26 AM
baloghadamsoftware added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
127–129

I think we should skip these cases similarly to initialization inside a try block in a body that is compound statement. Maybe the initialization throws thus it is inappropriate to convert it to member initializer. I also added a test case for this.

baloghadamsoftware marked an inline comment as done.

Fix from CPlusPlus2a to CPlusPlus20 after rebase.

Code reformatted.

aaron.ballman added inline comments.May 30 2020, 10:02 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
107–108

This should now be done by overriding bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;, see bugprone/ThrowKeywordMissingCheck.h for an example.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-member-initializer.rst
4

You should probably link to the rule from somewhere in these docs.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer-assignment.cpp
30

By my reading of the core guideline (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c48-prefer-in-class-initializers-to-member-initializers-in-constructors-for-constant-initializers), it looks like n should also be diagnosed because all of the constructors in the class initialize the member to the same constant value. Is there a reason to deviate from the rule (or have I missed something)?

Also, I'd like to see a test case like:

class C {
  int n;
public:
  C() { n = 0; }
  explicit C(int) { n = 12; }
};

Updated according to the comments.

baloghadamsoftware marked 4 inline comments as done.Jun 2 2020, 3:31 AM
baloghadamsoftware added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer-assignment.cpp
30

This check only cares for initializations inside the body (rule C.49, but if the proper fix is to convert them to default member initializer according to rule C.48 then we follow that rule in the fix). For initializations implemented as constructor member initializers but according to C.48 they should have been implemented as default member initializers we already have check modernize-use-default-member-init.

aaron.ballman added inline comments.Jun 16 2020, 11:06 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer-assignment.cpp
30

Thank you for the explanation. I have the feeling that these two rules are going to have some really weird interactions together. For instance, the example you added at my request shows behavior I can't easily explain as a user:

class Complex19 {
  int n;
  // CHECK-FIXES: int n{0};
public:
  Complex19() {
    n = 0;
    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'n' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer]
    // CHECK-FIXES: {{^\ *$}}
  }

  explicit Complex19(int) {
    // CHECK-FIXES: Complex19(int) : n(12) {
    n = 12;
    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'n' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
    // CHECK-FIXES: {{^\ *$}}
  }

  ~Complex19() = default;
};

Despite both constructors having the assignment expression n = <literal>; one gets one diagnostic + fixit and the other gets a different diagnostic + fixit.

Also, from reading C.49, I don't see anything about using in-class initialization. I see C.49 as being about avoiding the situation where the ctor runs a constructor for a data member only to then immediately in the ctor body make an assignment to that member. You don't need to initialize then reinitialize when you can use a member init list to construct the object properly initially.

baloghadamsoftware marked 2 inline comments as done.Jun 24 2020, 6:10 AM
baloghadamsoftware added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer-assignment.cpp
30

Thank you for your comment. While I can agree with you, I think that this check should be fully consistent with modernize-use-default-member-init. Thus I tried it on the following example:

class C {
  int n;
public:
  C() : n(1) {}
  C(int nn) : n(nn) {}

  int get_n() { return n; }
};

I got the following message from Clang-Tidy:

warning: use default member initializer for 'n' [modernize-use-default-member-init]
  int n;
      ^
       {1}

This is no surprise, however. If you take a look on the example in C.48: Prefer in-class initializers to member initializers in constructors for constant initializers you can see that our code is almost the same as the example considered bad there. So rule C.49 does not speak anything about in-class initialization, but C.48 does, our check enforcing C.49 must not suggest a fixit the the check enforcing C.48 fixes again. We should not force the user to run Clang-Tidy in multiple passes. Maybe we can mention the numbers of the rules in the error messages to make them clearer.

aaron.ballman added inline comments.Jul 1 2020, 4:07 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer-assignment.cpp
30

This is no surprise, however.

Agreed, but it's also a different example from the one I posted where the behavior is mysterious.

We should not force the user to run Clang-Tidy in multiple passes.

Agreed, but the checks are also independent of one another (e.g., I can run one but not the other) and that use case needs to be considered as well as the combined use case.

I personally don't use the C++ Core Guidelines and so I'm not as well-versed in their nuances -- I'm happy to let someone more familiar with the standard sway my opinion, but this smells a bit like the two rules are less orthogonal than the rule authors may believe. Both rules say to prefer something regarding member initialization, but neither rule says what the priority ordering is for those preferences when either approach works. Is it implicitly assumed that lower-numbered rules take precedence and so we should prefer in-class initialization over member initialization over assignment (that would seem defensible, but isn't stated anywhere in the rules)?

Just my 2 cents, but would it not be wise to introduce a test case that runs the 2 aforementioned checks together demonstrating how they interact with each other. This will also force compliance if either check is updated down the line.

baloghadamsoftware marked an inline comment as done.Jul 2 2020, 12:31 AM

Just my 2 cents, but would it not be wise to introduce a test case that runs the 2 aforementioned checks together demonstrating how they interact with each other. This will also force compliance if either check is updated down the line.

I can do that, and I will do it. However, all checks work on the original files, thus no interaction is expected.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer-assignment.cpp
30

Agreed, but it's also a different example from the one I posted where the behavior is mysterious.

The example in C.48 is the result of the fixit of this check for C.49 if I do not take C.48 in account. Thus without considering C.48 the check would suggest fixits which is bad according to C.48. This means that if the user accepts these fixits and runs Clang-Tidy again then the other check marks these fixits as bad and suggests another fixit for them.

The big question is that which one is the majority? Users who wish to conform to all the core guidelines or users who accept C.49 but not C.48. My wild guess is the first one. So in my opinion the biggest problem is that if we suggest a fixit by one check considered as bad by another one than the problem that we suggest different fixits for initializations in the default constructors than in other constructors.

Of course, the ideal solution would be to define the execution order of the checks and every check should work on a code fixed by the previous check. I do not think this would ever happen. It could also help if I could examine in this check whether the other one is enabled. However, I do not think this is possible either.

aaron.ballman added inline comments.Jul 2 2020, 4:57 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer-assignment.cpp
30

The example in C.48 is the result of the fixit of this check for C.49 if I do not take C.48 in account. Thus without considering C.48 the check would suggest fixits which is bad according to C.48. This means that if the user accepts these fixits and runs Clang-Tidy again then the other check marks these fixits as bad and suggests another fixit for them.

Yup, and that's the situation I'm worried about.

The big question is that which one is the majority? Users who wish to conform to all the core guidelines or users who accept C.49 but not C.48. My wild guess is the first one. So in my opinion the biggest problem is that if we suggest a fixit by one check considered as bad by another one than the problem that we suggest different fixits for initializations in the default constructors than in other constructors.

My experience has been that users frequently disable individual warning flags and checks, but as a reaction to a poor-quality diagnostic in the project, and so they usually leave the entire group of checks enabled at first. So I'd say both are likely situations.

It could also help if I could examine in this check whether the other one is enabled. However, I do not think this is possible either.

This is possible, but it's something we should really stop and think about before we decide to open the floodgates because I can imagine the testing matrix for these sort of interactions getting out of control. e.g., when this check is enabled, but not if it's spelled using that alias, then this other check behaves differently, but it may depend on options set for the first check, etc. But you can use ClangTidyContext::isCheckEnabled() to test this.

Since no better idea cam to my mind, in this version I check whether modernize-use-default-member-init is enabled. If it is, then we issue a warning and suggest a fix that uses default member initializer. We also take into account the option for that checker whether we should use brackets or assignment. This checker has no options now. If the other checker is not enabled, we always warn and suggest fix to use constructor member initializer.

aaron.ballman accepted this revision.Aug 13 2020, 11:04 AM

Since no better idea cam to my mind, in this version I check whether modernize-use-default-member-init is enabled. If it is, then we issue a warning and suggest a fix that uses default member initializer. We also take into account the option for that checker whether we should use brackets or assignment. This checker has no options now. If the other checker is not enabled, we always warn and suggest fix to use constructor member initializer.

While I don't feel awesome about this approach, I don't have a better idea either. Unless @alexfh, @gribozavr2, or one of the other reviewers has concerns, this LGTM. Thank you for your patience while we thought our way through this!

This revision is now accepted and ready to land.Aug 13 2020, 11:04 AM
mgehre removed a subscriber: mgehre.Aug 13 2020, 12:04 PM
njames93 added inline comments.Aug 13 2020, 2:06 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
104

OptionsView::get(...) is specialized for bools, so you can just pass false as the second argument and negate the need to compare against zero

Updated according to a comment.

baloghadamsoftware marked an inline comment as done.Aug 14 2020, 8:09 AM
lebedev.ri reopened this revision.EditedSep 10 2020, 6:34 AM
lebedev.ri added a subscriber: lebedev.ri.

So i've just reverted this in rGebf496d805521b53022a351f35854de977fee844.

@aaron.ballman @baloghadamsoftware how's QC going on nowadays here?
Was this evaluated on anything other than it's tests?

It appears to be either unacceptably slow, or subtly broken, because it takes at least 100x time more than all of the other clang-tidy checks enabled, and e.g.
clang-tidy-12 --checks="-*,cppcoreguidelines-prefer-member-initializer" -p . /repositories/llvm-project/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
never finishes (minutes and counting).

This revision is now accepted and ready to land.Sep 10 2020, 6:34 AM
lebedev.ri requested changes to this revision.Sep 10 2020, 6:35 AM
This revision now requires changes to proceed.Sep 10 2020, 6:35 AM

So i've just reverted this in rGebf496d805521b53022a351f35854de977fee844.

@aaron.ballman @baloghadamsoftware how's QC going on nowadays here?
Was this evaluated on anything other than it's tests?

Surely. After I commit a patch, lots of buildbots verify it. They passed so far.

It appears to be either unacceptably slow, or subtly broken, because it takes at least 100x time more than all of the other clang-tidy checks enabled, and e.g.
clang-tidy-12 --checks="-*,cppcoreguidelines-prefer-member-initializer" -p . /repositories/llvm-project/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
never finishes (minutes and counting).

I see nothing there that could be slow, thus this is probably some hang. I will investigate it.

So i've just reverted this in rGebf496d805521b53022a351f35854de977fee844.

@aaron.ballman @baloghadamsoftware how's QC going on nowadays here?
Was this evaluated on anything other than it's tests?

Surely. After I commit a patch, lots of buildbots verify it. They passed so far.

@baloghadamsoftware, i think you understand that wasn't the question.

It appears to be either unacceptably slow, or subtly broken, because it takes at least 100x time more than all of the other clang-tidy checks enabled, and e.g.
clang-tidy-12 --checks="-*,cppcoreguidelines-prefer-member-initializer" -p . /repositories/llvm-project/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
never finishes (minutes and counting).

I see nothing there that could be slow, thus this is probably some hang. I will investigate it.

So i've just reverted this in rGebf496d805521b53022a351f35854de977fee844.

@aaron.ballman @baloghadamsoftware how's QC going on nowadays here?
Was this evaluated on anything other than it's tests?

Surely. After I commit a patch, lots of buildbots verify it. They passed so far.

@baloghadamsoftware, i think you understand that wasn't the question.

This feedback is a bit terse and not very constructive. FWIW, we don't typically ask patch authors to run their patch over a large corpus of code unless a reviewer expects there to be a performance concern and asks explicitly. Given that this checks constructor bodies, there was no obvious reason to ask for that here. Also, I can't recall a time when we expected a patch reviewer to do that work. I appreciate that you noticed an issue and reverted so we could investigate, but when reporting an issue like this, please try to keep in mind that we're all in the same community trying to make a great product.

I found the problem: hasBody() always returns true if the function has somewhere a body. This means that also the hasBody matcher matches forward declarations. However, I only need the definition. This far I could not find any method to achieve that: isDefinition() also returns true for forward declarations, as well as comparing with the CanonicalDecl. I am looking further...

OK, I reversed the matcher expression now, it seems to work, but I will check it on the LLVM/Clang codebase first.

It looks like it was not entirely my fault: D87527.

Thank you for looking into it.

Minor inline comments.

Could you please add a test case where the constructor is defined out of line? So far in every test, the constructor body was inside the class. Something like:

struct S {
  int i, j;
  S();
};

S::S() {
  i = 1;
  j = 2;
}

I'm also particularly interested in the case when the constructor is implemented in its "own" translation unit. Where will the initialiser be moved in that case? Into the header, where the record is defined, still?

clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
22–24

It was you who figured out in D85728 that isa has changed recently and is now variadic. Then this function can be simplified with the variadic version.

40–42

Ditto.

127
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-member-initializer.rst
7
whisperity added inline comments.Sep 14 2020, 2:51 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
44

@baloghadamsoftware The original comment by Aaron is left unmarked as "done" here.

No crash, no hang, no false positives on the LLVM Project.

Updated according to the comments.

Prerequeisite patch is committed, the check is tested now on the LLVM Project. @lebedev.ri, @aaron.ballman can I recommit it?

lebedev.ri resigned from this revision.Sep 21 2020, 4:33 AM

Prerequeisite patch is committed, the check is tested now on the LLVM Project. @lebedev.ri, @aaron.ballman can I recommit it?

Thank you! SGTM.

This revision is now accepted and ready to land.Sep 21 2020, 4:33 AM
Rechi added a subscriber: Rechi.Sep 24 2020, 5:17 AM
Rechi added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
185

The following example generates invalid fixes, if modernize-use-default-member-init check isn't enabled, because Ctor->getNumCtorInitializers() returns 1.

class Example
{
public:
  Example() { a = 0; };
  int a;
  std::string string;
};
njames93 added inline comments.Feb 20 2021, 6:33 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
185

I've done a little work trying to fix some issues I've noticed while running this check in the wild. I have a feeling this shortcoming has been addressed in there - D97132.