[PATCH] D22642: CodeGen: Clean up implementation of vtable initializer builder. NFC.

[Richard Smith via cfe-commits]

rsmith accepted this revision.
This revision is now accepted and ready to land.

================
Comment at: include/clang/AST/VTableBuilder.h:222-225
@@ -221,6 +221,6 @@
 
   typedef const VTableComponent *vtable_component_iterator;
   typedef const VTableThunkTy *vtable_thunk_iterator;
   typedef llvm::iterator_range<vtable_component_iterator>
       vtable_component_range;
 
----------------
Can you remove these? It looks like they should be unused now.

================
Comment at: lib/CodeGen/CGVTables.cpp:527-528
@@ +526,4 @@
+  auto OffsetConstant = [&](CharUnits Offset) {
+    llvm::Type *PtrDiffTy =
+        CGM.getTypes().ConvertType(CGM.getContext().getPointerDiffType());
+    return llvm::ConstantExpr::getIntToPtr(
----------------
This is just `CGM.PtrDiffTy`.

================
Comment at: lib/CodeGen/CGVTables.cpp:588
@@ +587,3 @@
+        if (auto *F = dyn_cast<llvm::Function>(Cache))
+          F->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+        Cache = llvm::ConstantExpr::getBitCast(Cache, CGM.Int8PtrTy);
----------------
Do you have any idea why we're doing this? It looks wrong to me. These ABI entry points are exposed and could certainly have their addresses taken and used in this translation unit.

================
Comment at: lib/CodeGen/CGVTables.cpp:598-615
@@ -638,5 +597,20 @@
+                              CGM.getCXXABI().GetPureVirtualCallName());
+    } else if (cast<CXXMethodDecl>(GD.getDecl())->isDeleted()) {
+      return SpecialVirtualFn(DeletedVirtualFn,
+                              CGM.getCXXABI().GetDeletedVirtualCallName());
+    } else {
+      // Check if we should use a thunk.
+      if (NextVTableThunkIndex < VTLayout.vtable_thunks().size() &&
+          VTLayout.vtable_thunks()[NextVTableThunkIndex].first == Idx) {
+        const ThunkInfo &Thunk =
+            VTLayout.vtable_thunks()[NextVTableThunkIndex].second;
+
+        maybeEmitThunkForVTable(GD, Thunk);
+        NextVTableThunkIndex++;
+        return CGM.GetAddrOfThunk(GD, Thunk);
+      } else {
+        llvm::Type *Ty = CGM.getTypes().GetFunctionTypeForVTable(GD);
 
-        Init = llvm::ConstantExpr::getBitCast(Init, Int8PtrTy);
+        return CGM.GetAddrOfFunction(GD, Ty, /*ForVTable=*/true);
       }
     }
----------------
Please remove the `else`-after-`return` throughout here. That'll make it much more obvious to a reader of the code that fallthrough to the next `case` is not possible.

================
Comment at: lib/CodeGen/CGVTables.h:52
@@ -51,1 +51,3 @@
 
+  llvm::Constant *PureVirtualFn = nullptr, *DeletedVirtualFn = nullptr;
+
----------------
I'd prefer these to be two separate declarations, with doc comments.


https://reviews.llvm.org/D22642