[libcxx-commits] [PATCH] D72959: Relative VTables ABI on Fuchsia

[Leonard Chan via Phabricator via libcxx-commits]

leonardchan added inline comments.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:646
+
+    // Take offset from stub to the vtable component.
+    llvm::Function *HiddenFunc = M.getFunction(StubName);
----------------
rjmccall wrote:
> That's not what this block is doing.
> 
> I think it would make sense to build this stub in a helper function.
> 
> Can you just avoid making the stub entirely if the function is known to be defined in this translation unit (which will include all the internal-linkage cases, but also things like `hidden linkonce_odr`)?
Done and added a test for it. Was planning to add this later as a followup but it turned out to be pretty simple to implement.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:895
+    size_t thisIndex = layout.getVTableOffset(VTableIdx);
+    size_t nextIndex = thisIndex + layout.getVTableSize(VTableIdx);
+    unsigned LastAddressPoint = thisIndex;
----------------
rjmccall wrote:
> Please use the same local-variable capitalization conventions as the existing code, and please don't recompute `getNumVTables()` each time through the loop.
> 
> I agree that `vtableIndex` is a clearer name than `i`, but if you're going to rename locals for readability, please also rename `thisIndex` and `nextIndex`.
Updated naming in `addVTableComponent` and `addRelativeComponent` also.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72959/new/

https://reviews.llvm.org/D72959