[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor

[Aaron Ballman via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: clang-tidy/readability/RedundantMemberInitCheck.cpp:57
            "initializer for base class %0 is redundant")
-          << Init->getTypeSourceInfo()->getType()
+          << Construct->getType()
           << FixItHint::CreateRemoval(Init->getSourceRange());
----------------
malcolm.parsons wrote:
> aaron.ballman wrote:
> > malcolm.parsons wrote:
> > > malcolm.parsons wrote:
> > > > aaron.ballman wrote:
> > > > > malcolm.parsons wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Why is it more correct to use the CXXConstructExpr type information rather than the CXXCtorInitializer?
> > > > > > Something to do with templates and namespaces.
> > > > > > 
> > > > > > In the bug report, `CXXCtorInitializer` had type `std::__1::bitset<128>` and `CXXConstructExpr` had type `std::bitset<MAX_SUBTARGET_FEATURES>`.
> > > > > > 
> > > > > > I don't know why.
> > > > > I believe it's because `__1` is an inline namespace, and the printing policy matters. IIRC, there's the `SuppressUnwrittenScope` policy data member, that if you set it to true, it won't print the inline or anonymous namespace when printing types.
> > > > > 
> > > > > We should understand why there's a difference before applying this change. I think using the CXXCtorInitializer's type is more correct than using the CXXConstructExpr's type (due to implicit type conversions). Given that the printing policy controls whether inline namespaces are printed, I would have expected these both to print without the inline namespace (the type changed, but not the printing policy) -- the fact that the behavior differs makes me worried there's a bug somewhere else and this fix is masking it.
> > > > The difference isn't just the scope; `MAX_SUBTARGET_FEATURES` became `128` too.
> > > > 
> > > > Looking at `Sema::BuildMemInitializer()` didn't help me.
> > > The lookup of the base type has a Path with sugared type, but the Decl found has a canonical type.
> > So then you get the same behavior by getting the canonical type from `Init->getTypeSourceInfo()->getType()`?
> The AST is:
> 
> ```
> |-CXXRecordDecl 0x3748ef8 <line:102:1, line:106:1> line:102:8 struct F8 definition
> | |-public 'Foo::Template<N_THINGS>':'struct Foo::Bar::Template<5>'
> | |-CXXRecordDecl 0x3749508 <col:1, col:8> col:8 implicit referenced struct F8
> | `-CXXConstructorDecl 0x37495e0 <line:103:3, col:22> col:3 F8 'void (void)'
> |   |-CXXCtorInitializer 'struct Foo::Bar::Template<5>'
> |   | `-CXXConstructExpr 0x374b518 <col:10, col:19> 'Foo::Template<N_THINGS>':'struct Foo::Bar::Template<5>' 'void (void) throw()'
> |   `-CompoundStmt 0x374b578 <col:21, col:22>
> ```
> 
> `Init->getTypeSourceInfo()->getType()` is `Foo::Bar::Template<5>`
> `Init->getTypeSourceInfo()->getType().getCanonicalType()` is `Foo::Bar::Template<5>`
> `Construct->getType()` is `Foo::Template<N_THINGS>`
> `Construct->getType().getCanonicalType()` is `Foo::Bar::Template<5>`
> 
> I don't think there can be an implicit cast when constructing a base class.
Ah, okay, I think I see what's going on now! Thank you for helping me to get there. ;-) I think this change is correct.


https://reviews.llvm.org/D26118