Skip to content

Use range-based for to iterate over fields in record layout, NFC #122029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Use range-based for to iterate over fields in record layout, NFC #122029

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1824674
Use range-based for to iterate over fields in record layout, NFC
rnk Jan 8, 2025
f2a6f10
Address comments to improve readability
rnk Jan 8, 2025
9954370
Fix formatting
rnk Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion clang/include/clang/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3115,7 +3115,19 @@ class FieldDecl : public DeclaratorDecl, public Mergeable<FieldDecl> {

/// Returns the index of this field within its record,
/// as appropriate for passing to ASTRecordLayout::getFieldOffset.
unsigned getFieldIndex() const;
unsigned getFieldIndex() const {
const FieldDecl *C = getCanonicalDecl();
if (C->CachedFieldIndex == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change here compared to the original implementation assumes getCanonicalDecl()->getCanonicalDecl() always equals to getCanonicalDecl().
This makes sense but just double checking that this is indeed the case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reasonably confident that that is the case, every canonical decl in the declaration chain is the same at any point in time. I think the surprising thing about canonical declarations is that they can change when modules are lazily loaded, and that has bit me in the past when attempting to apply attributes to canonical declarations.

C->setCachedFieldIndex();
assert(C->CachedFieldIndex);
return C->CachedFieldIndex - 1;
}

private:
/// Set CachedFieldIndex to the index of this field plus one.
void setCachedFieldIndex() const;

public:

/// Determines whether this field is mutable (C++ only).
bool isMutable() const { return Mutable; }
Expand Down
10 changes: 3 additions & 7 deletions clang/lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4651,12 +4651,9 @@ bool FieldDecl::isPotentiallyOverlapping() const {
return hasAttr<NoUniqueAddressAttr>() && getType()->getAsCXXRecordDecl();
}

unsigned FieldDecl::getFieldIndex() const {
const FieldDecl *Canonical = getCanonicalDecl();
if (Canonical != this)
return Canonical->getFieldIndex();

if (CachedFieldIndex) return CachedFieldIndex - 1;
void FieldDecl::setCachedFieldIndex() const {
assert(this == getCanonicalDecl() &&
"should be called on the canonical decl");

unsigned Index = 0;
const RecordDecl *RD = getParent()->getDefinition();
Expand All @@ -4670,7 +4667,6 @@ unsigned FieldDecl::getFieldIndex() const {
}

assert(CachedFieldIndex && "failed to find field in parent");
return CachedFieldIndex - 1;
}

SourceRange FieldDecl::getSourceRange() const {
Expand Down
73 changes: 29 additions & 44 deletions clang/lib/AST/RecordLayoutBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,8 @@ class EmptySubobjectMap {
}

CharUnits
getFieldOffset(const ASTRecordLayout &Layout, unsigned FieldNo) const {
uint64_t FieldOffset = Layout.getFieldOffset(FieldNo);
getFieldOffset(const ASTRecordLayout &Layout, const FieldDecl *Field) const {
uint64_t FieldOffset = Layout.getFieldOffset(Field->getFieldIndex());
assert(FieldOffset % CharWidth == 0 &&
"Field offset not at char boundary!");

Expand Down Expand Up @@ -298,14 +298,12 @@ EmptySubobjectMap::CanPlaceBaseSubobjectAtOffset(const BaseSubobjectInfo *Info,
}

// Traverse all member variables.
unsigned FieldNo = 0;
for (CXXRecordDecl::field_iterator I = Info->Class->field_begin(),
E = Info->Class->field_end(); I != E; ++I, ++FieldNo) {
if (I->isBitField())
for (const FieldDecl *Field : Info->Class->fields()) {
if (Field->isBitField())
continue;

CharUnits FieldOffset = Offset + getFieldOffset(Layout, FieldNo);
if (!CanPlaceFieldSubobjectAtOffset(*I, FieldOffset))
CharUnits FieldOffset = Offset + getFieldOffset(Layout, Field);
if (!CanPlaceFieldSubobjectAtOffset(Field, FieldOffset))
return false;
}

Expand Down Expand Up @@ -345,14 +343,12 @@ void EmptySubobjectMap::UpdateEmptyBaseSubobjects(const BaseSubobjectInfo *Info,
}

// Traverse all member variables.
unsigned FieldNo = 0;
for (CXXRecordDecl::field_iterator I = Info->Class->field_begin(),
E = Info->Class->field_end(); I != E; ++I, ++FieldNo) {
if (I->isBitField())
for (const FieldDecl *Field : Info->Class->fields()) {
if (Field->isBitField())
continue;

CharUnits FieldOffset = Offset + getFieldOffset(Layout, FieldNo);
UpdateEmptyFieldSubobjects(*I, FieldOffset, PlacingEmptyBase);
CharUnits FieldOffset = Offset + getFieldOffset(Layout, Field);
UpdateEmptyFieldSubobjects(Field, FieldOffset, PlacingEmptyBase);
}
}

Expand Down Expand Up @@ -410,15 +406,12 @@ EmptySubobjectMap::CanPlaceFieldSubobjectAtOffset(const CXXRecordDecl *RD,
}

// Traverse all member variables.
unsigned FieldNo = 0;
for (CXXRecordDecl::field_iterator I = RD->field_begin(), E = RD->field_end();
I != E; ++I, ++FieldNo) {
if (I->isBitField())
for (const FieldDecl *Field : RD->fields()) {
if (Field->isBitField())
continue;

CharUnits FieldOffset = Offset + getFieldOffset(Layout, FieldNo);

if (!CanPlaceFieldSubobjectAtOffset(*I, FieldOffset))
CharUnits FieldOffset = Offset + getFieldOffset(Layout, Field);
if (!CanPlaceFieldSubobjectAtOffset(Field, FieldOffset))
return false;
}

Expand Down Expand Up @@ -521,15 +514,12 @@ void EmptySubobjectMap::UpdateEmptyFieldSubobjects(
}

// Traverse all member variables.
unsigned FieldNo = 0;
for (CXXRecordDecl::field_iterator I = RD->field_begin(), E = RD->field_end();
I != E; ++I, ++FieldNo) {
if (I->isBitField())
for (const FieldDecl *Field : RD->fields()) {
if (Field->isBitField())
continue;

CharUnits FieldOffset = Offset + getFieldOffset(Layout, FieldNo);

UpdateEmptyFieldSubobjects(*I, FieldOffset, PlacingOverlappingField);
CharUnits FieldOffset = Offset + getFieldOffset(Layout, Field);
UpdateEmptyFieldSubobjects(Field, FieldOffset, PlacingOverlappingField);
}
}

Expand Down Expand Up @@ -1455,10 +1445,8 @@ void ItaniumRecordLayoutBuilder::LayoutFields(const RecordDecl *D) {
bool InsertExtraPadding = D->mayInsertExtraPadding(/*EmitRemark=*/true);
bool HasFlexibleArrayMember = D->hasFlexibleArrayMember();
for (auto I = D->field_begin(), End = D->field_end(); I != End; ++I) {
auto Next(I);
++Next;
LayoutField(*I,
InsertExtraPadding && (Next != End || !HasFlexibleArrayMember));
LayoutField(*I, InsertExtraPadding &&
(std::next(I) != End || !HasFlexibleArrayMember));
}
}

Expand Down Expand Up @@ -3672,35 +3660,32 @@ static void DumpRecordLayout(raw_ostream &OS, const RecordDecl *RD,
}

// Dump fields.
uint64_t FieldNo = 0;
for (RecordDecl::field_iterator I = RD->field_begin(),
E = RD->field_end(); I != E; ++I, ++FieldNo) {
const FieldDecl &Field = **I;
uint64_t LocalFieldOffsetInBits = Layout.getFieldOffset(FieldNo);
for (const FieldDecl *Field : RD->fields()) {
uint64_t LocalFieldOffsetInBits = Layout.getFieldOffset(Field->getFieldIndex());
CharUnits FieldOffset =
Offset + C.toCharUnitsFromBits(LocalFieldOffsetInBits);

// Recursively dump fields of record type.
if (auto RT = Field.getType()->getAs<RecordType>()) {
if (auto RT = Field->getType()->getAs<RecordType>()) {
DumpRecordLayout(OS, RT->getDecl(), C, FieldOffset, IndentLevel,
Field.getName().data(),
Field->getName().data(),
/*PrintSizeInfo=*/false,
/*IncludeVirtualBases=*/true);
continue;
}

if (Field.isBitField()) {
if (Field->isBitField()) {
uint64_t LocalFieldByteOffsetInBits = C.toBits(FieldOffset - Offset);
unsigned Begin = LocalFieldOffsetInBits - LocalFieldByteOffsetInBits;
unsigned Width = Field.getBitWidthValue(C);
unsigned Width = Field->getBitWidthValue(C);
PrintBitFieldOffset(OS, FieldOffset, Begin, Width, IndentLevel);
} else {
PrintOffset(OS, FieldOffset, IndentLevel);
}
const QualType &FieldType = C.getLangOpts().DumpRecordLayoutsCanonical
? Field.getType().getCanonicalType()
: Field.getType();
OS << FieldType << ' ' << Field << '\n';
? Field->getType().getCanonicalType()
: Field->getType();
OS << FieldType << ' ' << *Field << '\n';
}

// Dump virtual bases.
Expand Down
6 changes: 3 additions & 3 deletions clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ struct CGRecordLowering {
llvm::Type *StorageType);
/// Lowers an ASTRecordLayout to a llvm type.
void lower(bool NonVirtualBaseType);
void lowerUnion(bool isNoUniqueAddress);
void lowerUnion(bool isNonVirtualBaseType);
void accumulateFields(bool isNonVirtualBaseType);
RecordDecl::field_iterator
accumulateBitFields(bool isNonVirtualBaseType,
Expand Down Expand Up @@ -310,9 +310,9 @@ void CGRecordLowering::lower(bool NVBaseType) {
computeVolatileBitfields();
}

void CGRecordLowering::lowerUnion(bool isNoUniqueAddress) {
void CGRecordLowering::lowerUnion(bool isNonVirtualBaseType) {
CharUnits LayoutSize =
isNoUniqueAddress ? Layout.getDataSize() : Layout.getSize();
isNonVirtualBaseType ? Layout.getDataSize() : Layout.getSize();
llvm::Type *StorageType = nullptr;
bool SeenNamedMember = false;
// Iterate through the fields setting bitFieldInfo and the Fields array. Also
Expand Down
Loading