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

rjmccall added a comment. I think this is okay. We can review further if we see other problems. https://reviews.llvm.org/D17893 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:4935-4943 @@ +4934,11 @@ + + bool HasStableAttr = Record->hasAttr(); + bool HasUnstableAttr = Record->hasAttr(); + if (HasStableAttr && HasUnstableAttr) { +Diag(Record->getLocation(),

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

joker.eph added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:4999 @@ +4998,3 @@ +return; + } + pcc wrote: > joker.eph wrote: > > joker.eph wrote: > > > Isn't this correct by the loop which starts with `DC = > > > InnermostExternalDC`? > >

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

pcc added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:4999 @@ +4998,3 @@ +return; + } + joker.eph wrote: > joker.eph wrote: > > Isn't this correct by the loop which starts with `DC = InnermostExternalDC`? > `s/correct/already covered/` That loop

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

pcc added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:4935-4943 @@ +4934,11 @@ + + bool HasStableAttr = Record->hasAttr(); + bool HasUnstableAttr = Record->hasAttr(); + if (HasStableAttr && HasUnstableAttr) { +Diag(Record->getLocation(), diag::err_abi_mismatch)

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

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>;

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

pcc added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:4935-4943 @@ +4934,11 @@ + + bool HasStableAttr = Record->hasAttr(); + bool HasUnstableAttr = Record->hasAttr(); + if (HasStableAttr && HasUnstableAttr) { +Diag(Record->getLocation(), diag::err_abi_mismatch)

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

aaron.ballman added a comment. Attribute side of things looks good, but I leave it to others to decide if the ABI functionality makes sense (it does to me, but I'm no ABI expert). http://reviews.llvm.org/D17893 ___ cfe-commits mailing list

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

joker.eph added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:4999 @@ +4998,3 @@ +return; + } + joker.eph wrote: > Isn't this correct by the loop which starts with `DC = InnermostExternalDC`? `s/correct/already covered/`

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

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:4935-4943 @@ +4934,11 @@ + + bool HasStableAttr = Record->hasAttr(); + bool HasUnstableAttr = Record->hasAttr(); + if (HasStableAttr && HasUnstableAttr) { +Diag(Record->getLocation(),

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

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>;

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

joker.eph added a comment. LGTM, but I'm not a clang expert, so if Richard or someone else could double-check? Comment at: lib/Sema/SemaDeclCXX.cpp:4999 @@ +4998,3 @@ +return; + } + Isn't this correct by the loop which starts with `DC =

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

pcc updated this revision to Diff 50668. pcc added a comment. - Add a test for passing arguments to the attribute http://reviews.llvm.org/D17893 Files: include/clang/AST/DeclCXX.h include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td

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

pcc added a comment. > Missing some attribute-related tests like attaching the attribute to > something other than a record, or passing arguments to the attribute. Added. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8345 @@ +8344,3 @@ +def

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

pcc updated this revision to Diff 50667. pcc marked 3 inline comments as done. pcc added a comment. - Address review comments http://reviews.llvm.org/D17893 Files: include/clang/AST/DeclCXX.h include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td

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

aaron.ballman added a comment. Missing some attribute-related tests like attaching the attribute to something other than a record, or passing arguments to the attribute. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8332 @@ +8331,3 @@ + "base %0 uses the stable

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

pcc added inline comments. Comment at: include/clang/Basic/Attr.td:1543 @@ +1542,3 @@ + let Subjects = SubjectList<[Record]>; + let Documentation = [Undocumented]; +} aaron.ballman wrote: > No new undocumented attributes, please. Same below. Yes, sorry, forgot

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

aaron.ballman added a subscriber: aaron.ballman. aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. I may have more comments when I catch up on email next week. Just a drive-by as I added myself as reviewer. Comment at: