[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor
malcolm.parsons created this revision.
malcolm.parsons added reviewers: aaron.ballman, Eugene.Zelenko.
malcolm.parsons added a subscriber: cfe-commits.
Fixes PR30835
https://reviews.llvm.org/D26118
Files:
clang-tidy/readability/RedundantMemberInitCheck.cpp
Index: clang-tidy/readability/RedundantMemberInitCheck.cpp
===
--- clang-tidy/readability/RedundantMemberInitCheck.cpp
+++ clang-tidy/readability/RedundantMemberInitCheck.cpp
@@ -54,7 +54,7 @@
} else {
diag(Init->getSourceLocation(),
"initializer for base class %0 is redundant")
- << Init->getTypeSourceInfo()->getType()
+ << Construct->getType()
<< FixItHint::CreateRemoval(Init->getSourceRange());
}
}
Index: clang-tidy/readability/RedundantMemberInitCheck.cpp
===
--- clang-tidy/readability/RedundantMemberInitCheck.cpp
+++ clang-tidy/readability/RedundantMemberInitCheck.cpp
@@ -54,7 +54,7 @@
} else {
diag(Init->getSourceLocation(),
"initializer for base class %0 is redundant")
- << Init->getTypeSourceInfo()->getType()
+ << Construct->getType()
<< FixItHint::CreateRemoval(Init->getSourceRange());
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor
aaron.ballman added a comment. This change is missing a test case. Comment at: clang-tidy/readability/RedundantMemberInitCheck.cpp:57 "initializer for base class %0 is redundant") - << Init->getTypeSourceInfo()->getType() + << Construct->getType() << FixItHint::CreateRemoval(Init->getSourceRange()); Why is it more correct to use the CXXConstructExpr type information rather than the CXXCtorInitializer? Repository: rL LLVM https://reviews.llvm.org/D26118 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor
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: > 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`. I don't know why. Repository: rL LLVM https://reviews.llvm.org/D26118 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor
aaron.ballman 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()); 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`. > > 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. Repository: rL LLVM https://reviews.llvm.org/D26118 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor
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: > > 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`. > > > > 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. Repository: rL LLVM https://reviews.llvm.org/D26118 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor
malcolm.parsons updated this revision to Diff 76797.
malcolm.parsons added a comment.
Add test.
https://reviews.llvm.org/D26118
Files:
clang-tidy/readability/RedundantMemberInitCheck.cpp
test/clang-tidy/readability-redundant-member-init.cpp
Index: test/clang-tidy/readability-redundant-member-init.cpp
===
--- test/clang-tidy/readability-redundant-member-init.cpp
+++ test/clang-tidy/readability-redundant-member-init.cpp
@@ -87,6 +87,24 @@
// CHECK-FIXES: F7() {}
};
+namespace Foo {
+inline namespace Bar {
+template
+struct Template {
+ Template() = default;
+ int i = N;
+};
+}
+}
+
+enum { N_THINGS = 5 };
+
+struct F8 : Foo::Template {
+ F8() : Template() {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: initializer for base class
'Foo::Template' is redundant
+ // CHECK-FIXES: F8() {}
+};
+
// Initializer not written
struct NF1 {
NF1() {}
Index: clang-tidy/readability/RedundantMemberInitCheck.cpp
===
--- clang-tidy/readability/RedundantMemberInitCheck.cpp
+++ clang-tidy/readability/RedundantMemberInitCheck.cpp
@@ -54,7 +54,7 @@
} else {
diag(Init->getSourceLocation(),
"initializer for base class %0 is redundant")
- << Init->getTypeSourceInfo()->getType()
+ << Construct->getType()
<< FixItHint::CreateRemoval(Init->getSourceRange());
}
}
Index: test/clang-tidy/readability-redundant-member-init.cpp
===
--- test/clang-tidy/readability-redundant-member-init.cpp
+++ test/clang-tidy/readability-redundant-member-init.cpp
@@ -87,6 +87,24 @@
// CHECK-FIXES: F7() {}
};
+namespace Foo {
+inline namespace Bar {
+template
+struct Template {
+ Template() = default;
+ int i = N;
+};
+}
+}
+
+enum { N_THINGS = 5 };
+
+struct F8 : Foo::Template {
+ F8() : Template() {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: initializer for base class 'Foo::Template' is redundant
+ // CHECK-FIXES: F8() {}
+};
+
// Initializer not written
struct NF1 {
NF1() {}
Index: clang-tidy/readability/RedundantMemberInitCheck.cpp
===
--- clang-tidy/readability/RedundantMemberInitCheck.cpp
+++ clang-tidy/readability/RedundantMemberInitCheck.cpp
@@ -54,7 +54,7 @@
} else {
diag(Init->getSourceLocation(),
"initializer for base class %0 is redundant")
- << Init->getTypeSourceInfo()->getType()
+ << Construct->getType()
<< FixItHint::CreateRemoval(Init->getSourceRange());
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor
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()); 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`. > > > > > > 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. https://reviews.llvm.org/D26118 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor
aaron.ballman 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()); 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`. > > > > > > > > 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()`? https://reviews.llvm.org/D26118 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor
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`. > > > > > > > > > > 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:8 struct F8 definition | |-public 'Foo::Template':'struct Foo::Bar::Template<5>' | |-CXXRecordDecl 0x3749508 col:8 implicit referenced struct F8 | `-CXXConstructorDecl 0x37495e0 col:3 F8 'void (void)' | |-CXXCtorInitializer 'struct Foo::Bar::Template<5>' | | `-CXXConstructExpr 0x374b518 'Foo::Template':'struct Foo::Bar::Template<5>' 'void (void) throw()' | `-CompoundStmt 0x374b578 ``` `Init->getTypeSourceInfo()->getType()` is `Foo::Bar::Template<5>` `Init->getTypeSourceInfo()->getType().getCanonicalType()` is `Foo::Bar::Template<5>` `Construct->getType()` is `Foo::Template` `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
[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor
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`. > > > > > > > > > > > > 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:8 struct F8 > definition > | |-public 'Foo::Template':'struct Foo::Bar::Template<5>' > | |-CXXRecordDecl 0x3749508 col:8 implicit referenced struct F8 > | `-CXXConstructorDecl 0x37495e0 col:3 F8 'void (void)' > | |-CXXCtorInitializer 'struct Foo::Bar::Template<5>' > | | `-CXXConstructExpr 0x374b518 > 'Foo::Template':'struct Foo::Bar::Template<5>' 'void (void) throw()' > | `-CompoundStmt 0x374b578 > ``` > > `Init->getTypeSourceInfo()->getType()` is `Foo::Bar::Template<5>` > `Init->getTypeSourceInfo()->getType().getCanonicalType()` is > `Foo::Bar::Template<5>` > `Construct->getType()` is `Foo::Template` > `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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26118: [clang-tidy] Change readability-redundant-member-init to get base type from constructor
This revision was automatically updated to reflect the committed changes.
Closed by commit rL286990: [clang-tidy] Change
readability-redundant-member-init to get base type from… (authored by
malcolm.parsons).
Changed prior to commit:
https://reviews.llvm.org/D26118?vs=76797&id=78024#toc
Repository:
rL LLVM
https://reviews.llvm.org/D26118
Files:
clang-tools-extra/trunk/clang-tidy/readability/RedundantMemberInitCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-member-init.cpp
Index:
clang-tools-extra/trunk/clang-tidy/readability/RedundantMemberInitCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/RedundantMemberInitCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/RedundantMemberInitCheck.cpp
@@ -54,7 +54,7 @@
} else {
diag(Init->getSourceLocation(),
"initializer for base class %0 is redundant")
- << Init->getTypeSourceInfo()->getType()
+ << Construct->getType()
<< FixItHint::CreateRemoval(Init->getSourceRange());
}
}
Index:
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-member-init.cpp
===
---
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-member-init.cpp
+++
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-member-init.cpp
@@ -87,6 +87,24 @@
// CHECK-FIXES: F7() {}
};
+namespace Foo {
+inline namespace Bar {
+template
+struct Template {
+ Template() = default;
+ int i = N;
+};
+}
+}
+
+enum { N_THINGS = 5 };
+
+struct F8 : Foo::Template {
+ F8() : Template() {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: initializer for base class
'Foo::Template' is redundant
+ // CHECK-FIXES: F8() {}
+};
+
// Initializer not written
struct NF1 {
NF1() {}
Index: clang-tools-extra/trunk/clang-tidy/readability/RedundantMemberInitCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/RedundantMemberInitCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/RedundantMemberInitCheck.cpp
@@ -54,7 +54,7 @@
} else {
diag(Init->getSourceLocation(),
"initializer for base class %0 is redundant")
- << Init->getTypeSourceInfo()->getType()
+ << Construct->getType()
<< FixItHint::CreateRemoval(Init->getSourceRange());
}
}
Index: clang-tools-extra/trunk/test/clang-tidy/readability-redundant-member-init.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/readability-redundant-member-init.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/readability-redundant-member-init.cpp
@@ -87,6 +87,24 @@
// CHECK-FIXES: F7() {}
};
+namespace Foo {
+inline namespace Bar {
+template
+struct Template {
+ Template() = default;
+ int i = N;
+};
+}
+}
+
+enum { N_THINGS = 5 };
+
+struct F8 : Foo::Template {
+ F8() : Template() {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: initializer for base class 'Foo::Template' is redundant
+ // CHECK-FIXES: F8() {}
+};
+
// Initializer not written
struct NF1 {
NF1() {}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
