[PATCH] D71199: [clang-tidy] New check cppcoreguidelines-prefer-member-initializer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer-assignment.cpp:29
+public:
+  Simple2() : n (0) {
+    x = 0.0;
----------------
baloghadamsoftware wrote:
> aaron.ballman wrote:
> > baloghadamsoftware wrote:
> > > aaron.ballman wrote:
> > > > baloghadamsoftware wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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; }
> > > > > > };
> > > > > > ```
> > > > > 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`.
> > > > 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.
> > > 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 [[ https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c48-prefer-in-class-initializers-to-member-initializers-in-constructors-for-constant-initializers | 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.
> > > 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)?
> > 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.
> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71199/new/

https://reviews.llvm.org/D71199