[PATCH] D152472: [Clang][MS] Remove assertion on BaseOffset can't be smaller than Size.

[Reid Kleckner via Phabricator via cfe-commits]

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