[PATCH] D22296: CodeGen: New vtable group representation: struct of vtable arrays.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.


================
Comment at: clang/include/clang/AST/VTableBuilder.h:225
 private:
+  std::vector<size_t> VTableIndices;
+
----------------
rjmccall wrote:
> Does this actually grow, or should you use a representation more like VTableComponents?  Or something custom that exploits the fact that, in the vast majority of cases, this vector will have size 1?
I wrapped up the code for VTableComponents and VTableThunks into a class, and used it here with some special handling for the size == 1 case.


================
Comment at: clang/include/clang/AST/VTableBuilder.h:260
+    assert(AddressPoint.second != 0 || IsMicrosoftABI);
     (void)IsMicrosoftABI;
 
----------------
rjmccall wrote:
> I know you didn't add this line, but could you remove this cast?  IsMicrosoftABI is not a local variable.
> 
> Also, the assert above suggests that this can be:
>   ... = AddressPoint.find(Base)->second;
> which should be slightly more efficient than lookup().  Also, I think the assertion becomes unnecessary at that point.
> Also, I think the assertion becomes unnecessary at that point.
Yes, and so does IsMicrosoftABI.

Also removed a similar assertion from one of the callers in CGVTT.cpp.


https://reviews.llvm.org/D22296