[PATCH] D18025: Add attributes for preserve_mostcc/preserve_allcc calling conventions to the C/C++ front-end

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 11 14:54:35 PST 2016


aaron.ballman added a comment.

In http://reviews.llvm.org/D18025#373373, @cfe-commits wrote:

> Could you review this simple patch? Is it OK to merge or are there any changes to be made?


The usual rule of thumb is to wait about a week before pinging a review -- sometimes the backlogs for reviews are kind of long. Also, all of the previous comments on the review appear to have gone missing, which makes the review kind of tough. It is possible to add cfe-commits to the subscribers list directly instead of starting an entirely new review (it's under the Action drop-down where you can add subscribers as well as reviewers).


================
Comment at: include/clang/Basic/Attr.td:1394
@@ -1393,1 +1393,3 @@
 
+def PreserveMost : InheritableAttr {
+  let Spellings = [GNU<"preserve_most">];
----------------
Do these attributes do anything on targets other than AArch64 and x86-64? If not, these should probably be inheriting from TargetSpecificAttr. (With tests confirming the behavior on other targets.)

================
Comment at: include/clang/Basic/AttrDocs.td:2137
@@ +2136,3 @@
+def PreserveMostDocs : Documentation {
+  let Category = DocCatVariable;
+  let Content = [{
----------------
This should go under DocCatCallingConvs instead of DocCatVariable (especially since calling conventions apply to functions, not variables, generally speaking).

================
Comment at: include/clang/Basic/AttrDocs.td:2165
@@ +2164,3 @@
+
+This calling convention will be used by a future version of the ObjectiveC
+runtime and should therefore still be considered experimental at this time.
----------------
Objective-C (here and elsewhere)?

================
Comment at: include/clang/Basic/AttrDocs.td:2176
@@ +2175,3 @@
+def PreserveAllDocs : Documentation {
+  let Category = DocCatVariable;
+  let Content = [{
----------------
Same category issue as above.

================
Comment at: test/CodeGen/preserve_most.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm < %s | FileCheck %s
+// RUN: %clang_cc1 -triple arm64-unknown-unknown -emit-llvm < %s | FileCheck %s
----------------
I would combine these tests with preserve_all.c and just have one file checking both attributes (since they're highly related).

================
Comment at: test/Sema/preserve_most-call-conv.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 %s -fsyntax-only -triple x86_64-unknown-unknown -verify
+// RUN: %clang_cc1 %s -fsyntax-only -triple arm64-unknown-unknown -verify
----------------
Same suggestion here as above.


http://reviews.llvm.org/D18025





More information about the cfe-commits mailing list