rnk added inline comments.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:3727
+ SmallVector<uint64_t, 4> Bases;
+ for (auto Base: Info.CXXInfo->BaseOffsets) {
+ Bases.push_back(Base.second.getQuantity());
----------------
Rather than iterating the map and sorting afterwards, IMO it's better to
iterate `RD->bases()` and look each base up in the map and use that offset. It
guarantees they'll be in the right order, even if the offsets aren't in
ascending order (although due to the ABI, they usually are).
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:3739
+ SmallVector<uint64_t, 4> VBases;
+ for (auto VBase: Info.CXXInfo->VBaseOffsets) {
+ VBases.push_back(VBase.second.VBaseOffset.getQuantity());
----------------
Ditto here, better to iterate over vbases().
================
Comment at: clang/lib/Frontend/LayoutOverrideSource.cpp:38
+ unsigned long long Offset = 0;
+ (void)S.substr(0, Idx).getAsInteger(10, Offset);
+ S = S.substr(Idx);
----------------
Can you pass in ULL directly here, rather than making a local and copying it
out?
================
Comment at: clang/lib/Frontend/LayoutOverrideSource.cpp:230
+ unsigned NumVB = 0;
+ for (const auto &I : RD->bases()) {
+ const CXXRecordDecl *Base = I.getType()->getAsCXXRecordDecl();
----------------
I think it's better to iterate `vbases()` directly, and then iterate bases()
and skip bases that are virtual.
================
Comment at: clang/test/CodeGenCXX/override-layout-ms.cpp:1
+// RUN: %clang_cc1 -w -triple=x86_64-pc-win32 -fms-compatibility
-fdump-record-layouts
-foverride-record-layout=%S/Inputs/override-layout-ms.layout %s | FileCheck %s
+
----------------
Go ahead and add a second RUN line without the "override-record-layout" flag to
confirm the layout is correct.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152472/new/
https://reviews.llvm.org/D152472
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
