baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware 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;
----------------
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71199/new/
https://reviews.llvm.org/D71199
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
