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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
