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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
