[PATCH] D58321: Support for relative vtables

[John McCall via Phabricator via llvm-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/AST/DeclCXX.h:531
+    /// \brief Whether the class uses the relative C++ vtable ABI.
+    unsigned IsRelativeCXXABI : 1;
+
----------------
Should we proactively generalize this as a "CXXABIVariant" enum, which for now can just be "Standard" and "RelativeVTables"?

Also, I don't want to pre-empt your secret plans, but if Fuchsia is just going to use this as its system C++ ABI, maybe we should plan for that, too.


================
Comment at: clang/include/clang/AST/VTableBuilder.h:267
 
+  VTableComponent &getVTableComponent(size_t i) const {
+    return VTableComponents[i];
----------------
`const VTableComponent &`, I think.


================
Comment at: clang/include/clang/Basic/LangOptions.def:329
+        "Whether to use clang's relative C++ ABI "
+        "for classes with vtables")
+
----------------
Yeah, see, this plays into the question above.  I would not want to provide this as a language option for general use.  The attribute seems good enough for testing, and if you want a -cc1 option to apply the attribute by default for experimentation during Fuchsia bring-up that's fair, but I don't want something that suggests to users that it's okay to pass this attribute and change the system default.


================
Comment at: clang/include/clang/Sema/Sema.h:10976
+  /// Determine if this class can use the relative vtable ABI.
+  void checkClassABI(CXXRecordDecl *RD);
+
----------------
Comment / method-name mismatch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58321