pcc 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">>;
----------------
rsmith wrote:
> 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.)
I agree that the attribute should permit other struct ABI changes. I also agree
that we shouldn't warn by default. We've traditionally warned when an attribute
has no effect, but that's probably the wrong decision in this case. I've made
this a non-default 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.
----------------
rsmith wrote:
> Can we exit early if no unstable class ABI flags were passed?
We may need to inherit an ABI from a base, even if no 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;
+ }
+
----------------
rsmith wrote:
> Should you also diagnose conflicts with other struct ABI attributes like
> `ms_struct`?
I'd expect those attributes to be orthogonal, e.g. `ms_struct` + `unstable_abi`
would give you whatever just `unstable_abi` would give you when targeting
Windows.
================
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()) {
----------------
rsmith wrote:
> `-Weverything` users will not like this.
That's unfortunate, but I think the alternative of creating a category of
warnings that `-Weverything` does not warn on would be worse. My understanding
is that as a user-facing flag `-Weverything` is mostly intended for manual
discovery of warnings that Clang produces; as part of that manual process,
users can easily add the `-Wno-c++-stable-abi` flag.
http://reviews.llvm.org/D17893
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
