[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:
> > > > 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)?


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

https://reviews.llvm.org/D71199