[PATCH] D10802: [mips] Interrupt attribute support.

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:255
@@ -254,1 +254,3 @@
 def err_parameter_name_omitted : Error<"parameter name omitted">;
+def err_excess_arguments : Error<"function %0 has too many arguments">;
+def err_wrong_return_type : Error<"function %0 does not have the \'void\' return type">;
----------------
I would combine these, and name the result so that it is obvious it relates to the MIPS interrupt attribute. This can then become: "MIPS 'interrupt' attribute only applies to functions %select{with no parameters|a 'void' return type}0" Generally speaking, we make these warnings instead of errors (though the choice is obviously yours), and add them to the IgnoredAttributes group (so you warn, and then never attach the attribute to the declaration).

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4441
@@ +4440,3 @@
+
+  DeclarationName Name = D->getAsFunction()->getNameInfo().getName();
+
----------------
No need for this; you can pass any NamedDecl * into Diag() and it will get quoted and displayed properly. Since you've already checked that it's a function or method, you can simply use cast<NamedDecl>(D) when needed.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4448
@@ +4447,3 @@
+
+  if (!(getFunctionOrMethodResultType(D)->isVoidType())) {
+    S.Diag(D->getLocation(), diag::err_wrong_return_type) << Name;
----------------
No need for the extra set of parens.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4453
@@ +4452,3 @@
+
+  if (D->hasAttr<Mips16Attr>()) {
+    S.Diag(Attr.getLoc(), diag::err_attributes_are_not_compatible)
----------------
Please use checkAttrMutualExclusion() instead. Also, you need to add a similar check to the Mips16 attribute handler to ensure the mutual exclusion is properly checked. Should have semantic test cases for both orderings:
```
__attribute__((mips16)) __attribute__((interrupt)) void f();
__attribute__((interrupt)) __attribute__((mips16)) void g();
```
What about the nomips16 attribute? Should this be mutually exclusive as well?


http://reviews.llvm.org/D10802