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

[malcolm.parsons]

Summary

Fixes PR30835

[aaron.ballman]

This change is missing a test case.

clang-tidy/readability/RedundantMemberInitCheck.cpp

57

Why is it more correct to use the CXXConstructExpr type information rather than the CXXCtorInitializer?

[malcolm.parsons]

clang-tidy/readability/RedundantMemberInitCheck.cpp

57

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.

[aaron.ballman]

clang-tidy/readability/RedundantMemberInitCheck.cpp

57

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.

[malcolm.parsons]

clang-tidy/readability/RedundantMemberInitCheck.cpp

57

The difference isn't just the scope; MAX_SUBTARGET_FEATURES became 128 too.

Looking at Sema::BuildMemInitializer() didn't help me.

[malcolm.parsons]

Add test.

[malcolm.parsons]

clang-tidy/readability/RedundantMemberInitCheck.cpp

57

The lookup of the base type has a Path with sugared type, but the Decl found has a canonical type.

[aaron.ballman]

clang-tidy/readability/RedundantMemberInitCheck.cpp

57

So then you get the same behavior by getting the canonical type from Init->getTypeSourceInfo()->getType()?

[malcolm.parsons]

clang-tidy/readability/RedundantMemberInitCheck.cpp

57

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.

[aaron.ballman]

LGTM!

clang-tidy/readability/RedundantMemberInitCheck.cpp

57

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.

clang-tidy/readability/RedundantMemberInitCheck.cpp

if (Construct->getNumArgs() == 0 ||
       Construct->getArg(0)->isDefaultArgument()) {
     if (Init->isAnyMemberInitializer()) {
       diag(Init->getSourceLocation(), "initializer for member %0 is redundant")
           << Init->getMember()
           << FixItHint::CreateRemoval(Init->getSourceRange());
     } else {
       diag(Init->getSourceLocation(),
            "initializer for base class %0 is redundant")
-          << Init->getTypeSourceInfo()->getType()
+          << Construct->getType()
           << FixItHint::CreateRemoval(Init->getSourceRange());
     }
   }
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang