leonardchan 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; + ---------------- pcc wrote: > rjmccall wrote: > > 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. > At this point I probably would remove this bitfield entirely. This > implementation does not support enabling the ABI on a per-class basis, so > everywhere that we are currently checking this field we should just be able > to check `RelativeCXXABIVTables` in `LangOptions`. The goal for us is to just enable this for all vtables so we don't need this on a per-class basis. Removed the bitfield. ================ Comment at: clang/include/clang/Basic/LangOptions.def:329 + "Whether to use clang's relative C++ ABI " + "for classes with vtables") + ---------------- rjmccall wrote: > 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. Ok. So is this the reason for the white list approach mentioned in D17893? As an alternative, would you also be ok with creating a `FuchsiaCXXABI` that subclasses `ItaniumCXXABI`, similar to the ARM and WebAssembly ones? This way it doesn't 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); + ---------------- rjmccall wrote: > Comment / method-name mismatch? Removed this method since the bitfield we set here is removed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58321/new/ https://reviews.llvm.org/D58321 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
