malcolm.parsons added inline comments.
================ Comment at: clang-tidy/readability/RedundantMemberInitCheck.cpp:57 "initializer for base class %0 is redundant") - << Init->getTypeSourceInfo()->getType() + << Construct->getType() << FixItHint::CreateRemoval(Init->getSourceRange()); ---------------- 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. https://reviews.llvm.org/D26118 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits