[Clang][CodeGen]NFC] Modernize loops in EmitCtorPrologue #155668
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch updates the loops in EmitCtorPrologue to use range-based for loops rather than looping over a single iterator which was being shared between three loops. Setting up three separate ranges adds a very small amount of overhead, but it improves the readability and maintainability of the code.
|
@llvm/pr-subscribers-clang-codegen Author: Andy Kaylor (andykaylor) ChangesThis patch updates the loops in EmitCtorPrologue to use range-based for loops rather than looping over a single iterator which was being shared between three loops. Setting up three separate ranges adds a very small amount of overhead, but it improves the readability and maintainability of the code. Full diff: https://github.com/llvm/llvm-project/pull/155668.diff 1 Files Affected:
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
index bae55aa1e1928..b4a6acc0f2d5f 100644
--- a/clang/lib/CodeGen/CGClass.cpp
+++ b/clang/lib/CodeGen/CGClass.cpp
@@ -1271,10 +1271,7 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
const CXXRecordDecl *ClassDecl = CD->getParent();
- CXXConstructorDecl::init_const_iterator B = CD->init_begin(),
- E = CD->init_end();
-
- // Virtual base initializers first, if any. They aren't needed if:
+ // Virtual base initializers aren't needed if:
// - This is a base ctor variant
// - There are no vbases
// - The class is abstract, so a complete object of it cannot be constructed
@@ -1296,15 +1293,36 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
assert(BaseCtorContinueBB);
}
- for (; B != E && (*B)->isBaseInitializer() && (*B)->isBaseVirtual(); B++) {
+ // Create three separate ranges for the different types of initializers.
+ auto AllInits = CD->inits();
+
+ // Find the boundaries between the three groups.
+ auto VirtualBaseEnd = std::find_if(
+ AllInits.begin(), AllInits.end(), [](const CXXCtorInitializer *Init) {
+ return !(Init->isBaseInitializer() && Init->isBaseVirtual());
+ });
+
+ auto NonVirtualBaseEnd = std::find_if(VirtualBaseEnd, AllInits.end(),
+ [](const CXXCtorInitializer *Init) {
+ return !Init->isBaseInitializer();
+ });
+
+ // Create the three ranges.
+ auto VirtualBaseInits = llvm::make_range(AllInits.begin(), VirtualBaseEnd);
+ auto NonVirtualBaseInits =
+ llvm::make_range(VirtualBaseEnd, NonVirtualBaseEnd);
+ auto MemberInits = llvm::make_range(NonVirtualBaseEnd, AllInits.end());
+
+ // Process virtual base initializers.
+ for (CXXCtorInitializer *Initializer : VirtualBaseInits) {
if (!ConstructVBases)
continue;
SaveAndRestore ThisRAII(CXXThisValue);
if (CGM.getCodeGenOpts().StrictVTablePointers &&
CGM.getCodeGenOpts().OptimizationLevel > 0 &&
- isInitializerOfDynamicClass(*B))
+ isInitializerOfDynamicClass(Initializer))
CXXThisValue = Builder.CreateLaunderInvariantGroup(LoadCXXThis());
- EmitBaseInitializer(*this, ClassDecl, *B);
+ EmitBaseInitializer(*this, ClassDecl, Initializer);
}
if (BaseCtorContinueBB) {
@@ -1314,14 +1332,14 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
}
// Then, non-virtual base initializers.
- for (; B != E && (*B)->isBaseInitializer(); B++) {
- assert(!(*B)->isBaseVirtual());
+ for (CXXCtorInitializer *Initializer : NonVirtualBaseInits) {
+ assert(!Initializer->isBaseVirtual());
SaveAndRestore ThisRAII(CXXThisValue);
if (CGM.getCodeGenOpts().StrictVTablePointers &&
CGM.getCodeGenOpts().OptimizationLevel > 0 &&
- isInitializerOfDynamicClass(*B))
+ isInitializerOfDynamicClass(Initializer))
CXXThisValue = Builder.CreateLaunderInvariantGroup(LoadCXXThis());
- EmitBaseInitializer(*this, ClassDecl, *B);
+ EmitBaseInitializer(*this, ClassDecl, Initializer);
}
InitializeVTablePointers(ClassDecl);
@@ -1329,8 +1347,7 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
// And finally, initialize class members.
FieldConstructionScope FCS(*this, LoadCXXThisAddress());
ConstructorMemcpyizer CM(*this, CD, Args);
- for (; B != E; B++) {
- CXXCtorInitializer *Member = (*B);
+ for (CXXCtorInitializer *Member : MemberInits) {
assert(!Member->isBaseInitializer());
assert(Member->isAnyMemberInitializer() &&
"Delegating initializer on non-delegating constructor");
|
|
@llvm/pr-subscribers-clang Author: Andy Kaylor (andykaylor) ChangesThis patch updates the loops in EmitCtorPrologue to use range-based for loops rather than looping over a single iterator which was being shared between three loops. Setting up three separate ranges adds a very small amount of overhead, but it improves the readability and maintainability of the code. Full diff: https://github.com/llvm/llvm-project/pull/155668.diff 1 Files Affected:
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
index bae55aa1e1928..b4a6acc0f2d5f 100644
--- a/clang/lib/CodeGen/CGClass.cpp
+++ b/clang/lib/CodeGen/CGClass.cpp
@@ -1271,10 +1271,7 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
const CXXRecordDecl *ClassDecl = CD->getParent();
- CXXConstructorDecl::init_const_iterator B = CD->init_begin(),
- E = CD->init_end();
-
- // Virtual base initializers first, if any. They aren't needed if:
+ // Virtual base initializers aren't needed if:
// - This is a base ctor variant
// - There are no vbases
// - The class is abstract, so a complete object of it cannot be constructed
@@ -1296,15 +1293,36 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
assert(BaseCtorContinueBB);
}
- for (; B != E && (*B)->isBaseInitializer() && (*B)->isBaseVirtual(); B++) {
+ // Create three separate ranges for the different types of initializers.
+ auto AllInits = CD->inits();
+
+ // Find the boundaries between the three groups.
+ auto VirtualBaseEnd = std::find_if(
+ AllInits.begin(), AllInits.end(), [](const CXXCtorInitializer *Init) {
+ return !(Init->isBaseInitializer() && Init->isBaseVirtual());
+ });
+
+ auto NonVirtualBaseEnd = std::find_if(VirtualBaseEnd, AllInits.end(),
+ [](const CXXCtorInitializer *Init) {
+ return !Init->isBaseInitializer();
+ });
+
+ // Create the three ranges.
+ auto VirtualBaseInits = llvm::make_range(AllInits.begin(), VirtualBaseEnd);
+ auto NonVirtualBaseInits =
+ llvm::make_range(VirtualBaseEnd, NonVirtualBaseEnd);
+ auto MemberInits = llvm::make_range(NonVirtualBaseEnd, AllInits.end());
+
+ // Process virtual base initializers.
+ for (CXXCtorInitializer *Initializer : VirtualBaseInits) {
if (!ConstructVBases)
continue;
SaveAndRestore ThisRAII(CXXThisValue);
if (CGM.getCodeGenOpts().StrictVTablePointers &&
CGM.getCodeGenOpts().OptimizationLevel > 0 &&
- isInitializerOfDynamicClass(*B))
+ isInitializerOfDynamicClass(Initializer))
CXXThisValue = Builder.CreateLaunderInvariantGroup(LoadCXXThis());
- EmitBaseInitializer(*this, ClassDecl, *B);
+ EmitBaseInitializer(*this, ClassDecl, Initializer);
}
if (BaseCtorContinueBB) {
@@ -1314,14 +1332,14 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
}
// Then, non-virtual base initializers.
- for (; B != E && (*B)->isBaseInitializer(); B++) {
- assert(!(*B)->isBaseVirtual());
+ for (CXXCtorInitializer *Initializer : NonVirtualBaseInits) {
+ assert(!Initializer->isBaseVirtual());
SaveAndRestore ThisRAII(CXXThisValue);
if (CGM.getCodeGenOpts().StrictVTablePointers &&
CGM.getCodeGenOpts().OptimizationLevel > 0 &&
- isInitializerOfDynamicClass(*B))
+ isInitializerOfDynamicClass(Initializer))
CXXThisValue = Builder.CreateLaunderInvariantGroup(LoadCXXThis());
- EmitBaseInitializer(*this, ClassDecl, *B);
+ EmitBaseInitializer(*this, ClassDecl, Initializer);
}
InitializeVTablePointers(ClassDecl);
@@ -1329,8 +1347,7 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
// And finally, initialize class members.
FieldConstructionScope FCS(*this, LoadCXXThisAddress());
ConstructorMemcpyizer CM(*this, CD, Args);
- for (; B != E; B++) {
- CXXCtorInitializer *Member = (*B);
+ for (CXXCtorInitializer *Member : MemberInits) {
assert(!Member->isBaseInitializer());
assert(Member->isAnyMemberInitializer() &&
"Delegating initializer on non-delegating constructor");
|
| auto VirtualBaseInits = llvm::make_range(AllInits.begin(), VirtualBaseEnd); | ||
| auto NonVirtualBaseInits = | ||
| llvm::make_range(VirtualBaseEnd, NonVirtualBaseEnd); | ||
| auto MemberInits = llvm::make_range(NonVirtualBaseEnd, AllInits.end()); |
There was a problem hiding this comment.
It feels like there should be some kind of utility function for splitting ranges like this, but I can't find it.
clang/lib/CodeGen/CGClass.cpp
Outdated
| auto MemberInits = llvm::make_range(NonVirtualBaseEnd, AllInits.end()); | ||
|
|
||
| // Process virtual base initializers. | ||
| for (CXXCtorInitializer *Initializer : VirtualBaseInits) { | ||
| if (!ConstructVBases) |
There was a problem hiding this comment.
Move the "if" outside the loop?
There was a problem hiding this comment.
Oh, of course! It was inside the loop before because we needed to advance the iterator, but with separate ranges, we can skip the loop entirely.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch updates the loops in EmitCtorPrologue to use range-based for loops rather than looping over a single iterator which was being shared between three loops. Setting up three separate ranges adds a very small amount of overhead, but it improves the readability and maintainability of the code.