[cfe-commits] r163078 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/AST/MicrosoftCXXABI.cpp lib/Sema/SemaDeclAttr.cpp lib/Sema/SemaType.cpp test/SemaCXX/member-pointer-ms.cpp

[John McCall]

On Sep 4, 2012, at 9:20 AM, João Matos wrote:
> You are right, I'm sorry for having commited this. I had already addressed John's concern, but used the wrong patch. I attached it for review.

Review is an iterative process;  if you have to substantially restructure a patch to address a review, you need to send out the new patch for further review.  For example, in this case, I have a lot more comments.

Please revert what you committed if you haven't already.

+class MsInheritanceAttr : InheritableAttr {
+}

MS is an initialism and should be preserved in all-caps;  i.e., this should be MSInheritanceAttr.

 def SingleInheritance : InheritableAttr {
-  let Spellings = [Declspec<"__single_inheritance">];
+  let Spellings = [MSTypespec<"__single_inheritance">];
 }
 
 def MultipleInheritance : InheritableAttr {
-  let Spellings = [Declspec<"__multiple_inheritance">];
+  let Spellings = [MSTypespec<"__multiple_inheritance">];
 }
 
 def VirtualInheritance : InheritableAttr {
-  let Spellings = [Declspec<"__virtual_inheritance">];
+  let Spellings = [MSTypespec<"__virtual_inheritance">];
 }

Did you mean to make all these inherit from MsInheritanceAttr?

+def err_ms_inheritance_already_declared : Error<
+  "ignored since inheritance model was already declared as '"
+  "%select{single|multiple|virtual}0'">;

"attribute ignored because inheritance was previously declared as %select{single|multiple|virtual}0"

+  if (RD->getTypeForDecl()->isIncompleteType()) {

This is just RD->isCompleteDefinition() (because RD already came from a type).

+    if (RD->hasAttr<SingleInheritanceAttr>())
+      return 1;
+    else if (RD->hasAttr<MultipleInheritanceAttr>())
+      return 2;
+    else
+      return 3;

If you set up the attribute class inheritance correctly, you can grab the
MSInheritanceAttr and then check its kind.

+static bool hasOtherInheritanceAttr(Decl *D, AttributeList::Kind Kind,
+    int& Existing) {
+  if (Kind != AttributeList::AT_SingleInheritance &&
+      D->hasAttr<SingleInheritanceAttr>()) {
+    Existing = 0;
+    return true;
+  }
+  else if (Kind != AttributeList::AT_MultipleInheritance &&
+      D->hasAttr<MultipleInheritanceAttr>()) {
+    Existing = 1;
+    return true;
+  }
+  else if (Kind != AttributeList::AT_VirtualInheritance &&
+      D->hasAttr<VirtualInheritanceAttr>()) {
+    Existing = 2;
+    return true;
+  }
+  return false;
+}

Similarly, this would be massively simplified by using attribute classes.

John.