[PATCH] D17893: Sema: Add semantic analysis for the C++ ABI stability attributes and whitelist.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8380
@@ +8379,3 @@
+def warn_unused_abi_stability_attr : Warning<
+  "unused C++ ABI stability attribute on non-dynamic class">,
+  InGroup<DiagGroup<"unused-stability-attr">>;
----------------
How valuable is it to warn on this? It seems like we may want `unstable_abi` to affect other things than virtual functions in future (for instance, we may want to apply the ARM ABI "return this from constructors" change to classes with unstable ABI, fix some subtle problems in the class layout algorithm, pass certain trivially-copyable structs in registers even though they're not POD for the purpose of layout, ...). If the design intent is that this only affects the virtual call portion of the ABI, then I think it has the wrong name.

(If someone asks for the unstable ABI for the class, and it happens that the unstable ABI is the same as the stable one, I don't think that warrants a warning.)

================
Comment at: lib/Sema/SemaDeclCXX.cpp:4891
@@ +4890,3 @@
+void Sema::checkClassABI(CXXRecordDecl *Record) {
+  // This can only be done accurately for non-dependent types, as the
+  // determination uses the class's bases, which may be dependent.
----------------
Can we exit early if no unstable class ABI flags were passed?

================
Comment at: lib/Sema/SemaDeclCXX.cpp:4935-4943
@@ +4934,11 @@
+
+  bool HasStableAttr = Record->hasAttr<StableABIAttr>();
+  bool HasUnstableAttr = Record->hasAttr<UnstableABIAttr>();
+  if (HasStableAttr && HasUnstableAttr) {
+    Diag(Record->getLocation(), diag::err_abi_mismatch) << Record;
+    Diag(Record->getAttr<StableABIAttr>()->getLocation(),
+         diag::note_abi_stability_attr) << /*Unstable=*/false;
+    Diag(Record->getAttr<UnstableABIAttr>()->getLocation(),
+         diag::note_abi_stability_attr) << /*Unstable=*/true;
+  }
+
----------------
Should you also diagnose conflicts with other struct ABI attributes like `ms_struct`?

================
Comment at: lib/Sema/SemaDeclCXX.cpp:5018-5019
@@ +5017,4 @@
+  if (!SourceMgr.isInSystemHeader(Record->getLocation())) {
+    Diag(Record->getLocation(), diag::warn_cxx_stable_abi)
+        << Record;
+    if (!getLangOpts().UnstableABIContextNamesPath.empty()) {
----------------
`-Weverything` users will not like this.


http://reviews.llvm.org/D17893