Require 'x' and 'y' as the parameter names of the iterators.

By requiring the iterators to be named exactly 'x' and 'y', we can simplify processing.  We can also improve the error messages.  Note that the error messages are not printed in strict increasing order.  If that's important, it's trivial to separate the search for IndexOfFirstIterator into its own function.

Change-Id: I71ebf4118154924d67af31c44dbdb07ed0f54a77

10 files changed, 161 insertions, 180 deletions

diff --git a/slang_rs_export_foreach.cpp b/slang_rs_export_foreach.cpp
index 1dff611..ba3ac3a 100644
--- a/slang_rs_export_foreach.cpp
+++ b/slang_rs_export_foreach.cpp
@@ -32,19 +32,6 @@
 
 namespace slang {
 
-namespace {
-
-static void ReportNameError(RSContext *Context, clang::ParmVarDecl const *PVD) {
-  slangAssert(Context && PVD);
-  Context->ReportError(PVD->getLocation(),
-                       "Duplicate parameter entry "
-                       "(by position/name): '%0'")
-      << PVD->getName();
-}
-
-}  // namespace
-
-
 // This function takes care of additional validation and construction of
 // parameters related to forEach_* reflection.
 bool RSExportForEach::validateAndConstructParams(
@@ -77,8 +64,8 @@ bool RSExportForEach::validateAndConstructParams(
   return valid;
 }
 
-bool RSExportForEach::validateAndConstructOldStyleParams(RSContext *Context,
-    const clang::FunctionDecl *FD) {
+bool RSExportForEach::validateAndConstructOldStyleParams(
+    RSContext *Context, const clang::FunctionDecl *FD) {
   slangAssert(Context && FD);
   // If numParams is 0, we already marked this as a graphics root().
   slangAssert(numParams > 0);
@@ -98,23 +85,49 @@ bool RSExportForEach::validateAndConstructOldStyleParams(RSContext *Context,
   // Validate remaining parameter types
   // TODO(all): Add support for LOD/face when we have them
 
-  size_t i = 0;
-  const clang::ParmVarDecl *PVD = FD->getParamDecl(i);
-  clang::QualType QT = PVD->getType().getCanonicalType();
+  size_t IndexOfFirstIterator = numParams;
+  valid |= validateIterationParameters(Context, FD, &IndexOfFirstIterator);
 
-  // Check for const T1 *in
-  if (QT->isPointerType() && QT->getPointeeType().isConstQualified()) {
-    mIn = PVD;
-    i++;  // advance parameter pointer
-  }
+  // Validate the non-iterator parameters, which should all be found before the
+  // first iterator.
+  for (size_t i = 0; i < IndexOfFirstIterator; i++) {
+    const clang::ParmVarDecl *PVD = FD->getParamDecl(i);
+    clang::QualType QT = PVD->getType().getCanonicalType();
 
-  // Check for T2 *out
-  if (i < numParams) {
-    PVD = FD->getParamDecl(i);
-    QT = PVD->getType().getCanonicalType();
-    if (QT->isPointerType() && !QT->getPointeeType().isConstQualified()) {
-      mOut = PVD;
-      i++;  // advance parameter pointer
+    if (!QT->isPointerType()) {
+      Context->ReportError(PVD->getLocation(),
+                           "Compute kernel %0() cannot have non-pointer "
+                           "parameters besides 'x' and 'y'. Parameter '%1' is "
+                           "of type: '%2'")
+          << FD->getName() << PVD->getName() << PVD->getType().getAsString();
+      valid = false;
+      continue;
+    }
+
+    // The only non-const pointer should be out.
+    if (!QT->getPointeeType().isConstQualified()) {
+      if (mOut == NULL) {
+        mOut = PVD;
+      } else {
+        Context->ReportError(PVD->getLocation(),
+                             "Compute kernel %0() can only have one non-const "
+                             "pointer parameter. Parameters '%1' and '%2' are "
+                             "both non-const.")
+            << FD->getName() << mOut->getName() << PVD->getName();
+        valid = false;
+      }
+    } else {
+      if (mIn == NULL && mOut == NULL) {
+        mIn = PVD;
+      } else if (mUsrData == NULL) {
+        mUsrData = PVD;
+      } else {
+        Context->ReportError(
+            PVD->getLocation(),
+            "Unexpected parameter '%0' for compute kernel %1()")
+            << PVD->getName() << FD->getName();
+        valid = false;
+      }
     }
   }
 
@@ -126,72 +139,11 @@ bool RSExportForEach::validateAndConstructOldStyleParams(RSContext *Context,
     valid = false;
   }
 
-  // Check for T3 *usrData
-  if (i < numParams) {
-    PVD = FD->getParamDecl(i);
-    QT = PVD->getType().getCanonicalType();
-    if (QT->isPointerType() && QT->getPointeeType().isConstQualified()) {
-      mUsrData = PVD;
-      i++;  // advance parameter pointer
-    }
-  }
-
-  while (i < numParams) {
-    PVD = FD->getParamDecl(i);
-    QT = PVD->getType().getCanonicalType();
-
-    if (QT.getUnqualifiedType() != C.UnsignedIntTy) {
-      Context->ReportError(
-          PVD->getLocation(),
-          "Unexpected kernel %0() parameter '%1' "
-          "of type '%2'")
-          << FD->getName() << PVD->getName() << PVD->getType().getAsString();
-      valid = false;
-    } else {
-      llvm::StringRef ParamName = PVD->getName();
-      if (ParamName.equals("x")) {
-        if (mX) {
-          ReportNameError(Context, PVD);
-          valid = false;
-        } else if (mY) {
-          // Can't go back to X after skipping Y
-          ReportNameError(Context, PVD);
-          valid = false;
-        } else {
-          mX = PVD;
-        }
-      } else if (ParamName.equals("y")) {
-        if (mY) {
-          ReportNameError(Context, PVD);
-          valid = false;
-        } else {
-          mY = PVD;
-        }
-      } else {
-        if (!mX && !mY) {
-          mX = PVD;
-        } else if (!mY) {
-          mY = PVD;
-        } else {
-          Context->ReportError(PVD->getLocation(),
-                               "Unexpected kernel %0() parameter '%1' "
-                               "of type '%2'")
-              << FD->getName() << PVD->getName()
-              << PVD->getType().getAsString();
-          valid = false;
-        }
-      }
-    }
-
-    i++;
-  }
-
   return valid;
 }
 
-
-bool RSExportForEach::validateAndConstructKernelParams(RSContext *Context,
-    const clang::FunctionDecl *FD) {
+bool RSExportForEach::validateAndConstructKernelParams(
+    RSContext *Context, const clang::FunctionDecl *FD) {
   slangAssert(Context && FD);
   bool valid = true;
   clang::ASTContext &C = Context->getASTContext();
@@ -208,12 +160,12 @@ bool RSExportForEach::validateAndConstructKernelParams(RSContext *Context,
 
   // Denote that we are indeed a pass-by-value kernel.
   mIsKernelStyle = true;
-
   mHasReturnType = (mResultType != C.VoidTy);
 
   if (mResultType->isPointerType()) {
-    Context->ReportError(FD->getTypeSpecStartLoc(),
-                         "Compute kernel %0() cannot return a pointer type: '%1'")
+    Context->ReportError(
+        FD->getTypeSpecStartLoc(),
+        "Compute kernel %0() cannot return a pointer type: '%1'")
         << FD->getName() << mResultType.getAsString();
     valid = false;
   }
@@ -221,35 +173,30 @@ bool RSExportForEach::validateAndConstructKernelParams(RSContext *Context,
   // Validate remaining parameter types
   // TODO(all): Add support for LOD/face when we have them
 
-  size_t i = 0;
-  const clang::ParmVarDecl *PVD = NULL;
-  clang::QualType QT;
-
-  if (i < numParams) {
-    PVD = FD->getParamDecl(i);
-    QT = PVD->getType().getCanonicalType();
+  size_t IndexOfFirstIterator = numParams;
+  valid |= validateIterationParameters(Context, FD, &IndexOfFirstIterator);
 
+  // Validate the non-iterator parameters, which should all be found before the
+  // first iterator.
+  for (size_t i = 0; i < IndexOfFirstIterator; i++) {
+    const clang::ParmVarDecl *PVD = FD->getParamDecl(i);
+    if (i == 0) {
+      mIn = PVD;
+    } else {
+      Context->ReportError(PVD->getLocation(),
+                           "Unrecognized parameter '%0'. Compute kernel %1() "
+                           "can only have one input parameter, 'x', and 'y'")
+          << PVD->getName() << FD->getName();
+      valid = false;
+    }
+    clang::QualType QT = PVD->getType().getCanonicalType();
     if (QT->isPointerType()) {
       Context->ReportError(PVD->getLocation(),
                            "Compute kernel %0() cannot have "
                            "parameter '%1' of pointer type: '%2'")
           << FD->getName() << PVD->getName() << PVD->getType().getAsString();
       valid = false;
-    } else if (QT.getUnqualifiedType() == C.UnsignedIntTy) {
-      // First parameter is either input or x, y (iff it is uint32_t).
-      llvm::StringRef ParamName = PVD->getName();
-      if (ParamName.equals("x")) {
-        mX = PVD;
-      } else if (ParamName.equals("y")) {
-        mY = PVD;
-      } else {
-        mIn = PVD;
-      }
-    } else {
-      mIn = PVD;
     }
-
-    i++;  // advance parameter pointer
   }
 
   // Check that we have at least one allocation to use for dimensions.
@@ -262,54 +209,63 @@ bool RSExportForEach::validateAndConstructKernelParams(RSContext *Context,
     valid = false;
   }
 
-  // TODO: Abstract this block away, since it is duplicate code.
-  while (i < numParams) {
-    PVD = FD->getParamDecl(i);
-    QT = PVD->getType().getCanonicalType();
+  return valid;
+}
 
-    if (QT.getUnqualifiedType() != C.UnsignedIntTy) {
-      Context->ReportError(PVD->getLocation(),
-                           "Unexpected kernel %0() parameter '%1' "
-                           "of type '%2'")
-          << FD->getName() << PVD->getName() << PVD->getType().getAsString();
-      valid = false;
+// Search for the optional x and y parameters.  Returns true if valid.   Also
+// sets *IndexOfFirstIterator to the index of the first iterator parameter, or
+// FD->getNumParams() if none are found.
+bool RSExportForEach::validateIterationParameters(
+    RSContext *Context, const clang::FunctionDecl *FD,
+    size_t *IndexOfFirstIterator) {
+  slangAssert(IndexOfFirstIterator != NULL);
+  slangAssert(mX == NULL && mY == NULL);
+  clang::ASTContext &C = Context->getASTContext();
+
+  // Find the x and y parameters if present.
+  size_t NumParams = FD->getNumParams();
+  *IndexOfFirstIterator = NumParams;
+  bool valid = true;
+  for (size_t i = 0; i < NumParams; i++) {
+    const clang::ParmVarDecl *PVD = FD->getParamDecl(i);
+    llvm::StringRef ParamName = PVD->getName();
+    if (ParamName.equals("x")) {
+      slangAssert(mX == NULL);  // We won't be invoked if two 'x' are present.
+      mX = PVD;
+      if (mY != NULL) {
+        Context->ReportError(PVD->getLocation(),
+                             "In compute kernel %0(), parameter 'x' should "
+                             "be defined before parameter 'y'")
+            << FD->getName();
+        valid = false;
+      }
+    } else if (ParamName.equals("y")) {
+      slangAssert(mY == NULL);  // We won't be invoked if two 'y' are present.
+      mY = PVD;
     } else {
-      llvm::StringRef ParamName = PVD->getName();
-      if (ParamName.equals("x")) {
-        if (mX) {
-          ReportNameError(Context, PVD);
-          valid = false;
-        } else if (mY) {
-          // Can't go back to X after skipping Y
-          ReportNameError(Context, PVD);
-          valid = false;
-        } else {
-          mX = PVD;
-        }
-      } else if (ParamName.equals("y")) {
-        if (mY) {
-          ReportNameError(Context, PVD);
-          valid = false;
-        } else {
-          mY = PVD;
-        }
-      } else {
-        if (!mX && !mY) {
-          mX = PVD;
-        } else if (!mY) {
-          mY = PVD;
-        } else {
-          Context->ReportError(PVD->getLocation(),
-                               "Unexpected kernel %0() parameter '%1' "
-                               "of type '%2'")
-              << FD->getName() << PVD->getName()
-              << PVD->getType().getAsString();
-          valid = false;
-        }
+      // It's neither x nor y.
+      if (*IndexOfFirstIterator < NumParams) {
+        Context->ReportError(PVD->getLocation(),
+                             "In compute kernel %0(), parameter '%1' cannot "
+                             "appear after the 'x' and 'y' parameters")
+            << FD->getName() << ParamName;
+        valid = false;
       }
+      continue;
+    }
+    // Validate the data type of x and y.
+    clang::QualType QT = PVD->getType().getCanonicalType();
+    if (QT.getUnqualifiedType() != C.UnsignedIntTy) {
+      Context->ReportError(
+          PVD->getLocation(),
+          "Parameter '%0' must be of type 'unsigned int'. It is of type '%1'")
+          << ParamName << PVD->getType().getAsString();
+      valid = false;
+    }
+    // If this is the first time we find an iterator, save it.
+    if (*IndexOfFirstIterator >= NumParams) {
+      *IndexOfFirstIterator = i;
     }
-
-    i++;  // advance parameter pointer
   }
   return valid;
 }
diff --git a/slang_rs_export_foreach.h b/slang_rs_export_foreach.h
index a2f9a02..be5db92 100644
--- a/slang_rs_export_foreach.h
+++ b/slang_rs_export_foreach.h
@@ -74,11 +74,15 @@ class RSExportForEach : public RSExportable {
                                   const clang::FunctionDecl *FD);
 
   bool validateAndConstructOldStyleParams(RSContext *Context,
-                                        const clang::FunctionDecl *FD);
+                                          const clang::FunctionDecl *FD);
 
   bool validateAndConstructKernelParams(RSContext *Context,
                                         const clang::FunctionDecl *FD);
 
+  bool validateIterationParameters(RSContext *Context,
+                                   const clang::FunctionDecl *FD,
+                                   size_t *IndexOfFirstIterator);
+
   bool setSignatureMetadata(RSContext *Context,
                             const clang::FunctionDecl *FD);
  public:
diff --git a/tests/F_kernel_noattr/kernel_noattr.rs b/tests/F_kernel_noattr/kernel_noattr.rs
index e46888b..27c5dcd 100644
--- a/tests/F_kernel_noattr/kernel_noattr.rs
+++ b/tests/F_kernel_noattr/kernel_noattr.rs
@@ -1,7 +1,7 @@
 #pragma version(1)
 #pragma rs java_package_name(foo)
 
-int root(uint32_t ain) {
+int root(uint32_t x) {
   return 0;
 }
 
diff --git a/tests/F_root_compute_int_in/stderr.txt.expect b/tests/F_root_compute_int_in/stderr.txt.expect
index 2c0bd7f..9cd6e25 100644
--- a/tests/F_root_compute_int_in/stderr.txt.expect
+++ b/tests/F_root_compute_int_in/stderr.txt.expect
@@ -1,2 +1,2 @@
+root_compute_int_in.rs:4:21: error: Compute kernel root() cannot have non-pointer parameters besides 'x' and 'y'. Parameter 'in' is of type: 'const int'
 root_compute_int_in.rs:4:6: error: Compute kernel root() must have at least one parameter for in or out
-root_compute_int_in.rs:4:21: error: Unexpected kernel root() parameter 'in' of type 'const int'
diff --git a/tests/F_root_compute_non_const_usrData/stderr.txt.expect b/tests/F_root_compute_non_const_usrData/stderr.txt.expect
index 6cafd50..3974ed7 100644
--- a/tests/F_root_compute_non_const_usrData/stderr.txt.expect
+++ b/tests/F_root_compute_non_const_usrData/stderr.txt.expect
@@ -1 +1 @@
-root_compute_non_const_usrData.rs:4:44: error: Unexpected kernel root() parameter 'usrData' of type 'void *'
+root_compute_non_const_usrData.rs:4:44: error: Compute kernel root() can only have one non-const pointer parameter. Parameters 'aout' and 'usrData' are both non-const.
diff --git a/tests/F_root_compute_non_ptr_usrData/stderr.txt.expect b/tests/F_root_compute_non_ptr_usrData/stderr.txt.expect
index 7bfda65..835e708 100644
--- a/tests/F_root_compute_non_ptr_usrData/stderr.txt.expect
+++ b/tests/F_root_compute_non_ptr_usrData/stderr.txt.expect
@@ -1 +1 @@
-root_compute_non_ptr_usrData.rs:4:48: error: Unexpected kernel root() parameter 'usrData' of type 'const int'
+root_compute_non_ptr_usrData.rs:4:48: error: Compute kernel root() cannot have non-pointer parameters besides 'x' and 'y'. Parameter 'usrData' is of type: 'const int'
diff --git a/tests/F_root_compute_non_uint32_t_xyzar/stderr.txt.expect b/tests/F_root_compute_non_uint32_t_xyzar/stderr.txt.expect
index 5c24e8c..40a6c67 100644
--- a/tests/F_root_compute_non_uint32_t_xyzar/stderr.txt.expect
+++ b/tests/F_root_compute_non_uint32_t_xyzar/stderr.txt.expect
@@ -1,4 +1,4 @@
-root_compute_non_uint32_t_xyzar.rs:5:15: error: Unexpected kernel root() parameter 'x' of type 'int'
-root_compute_non_uint32_t_xyzar.rs:5:24: error: Unexpected kernel root() parameter 'y' of type 'float'
-root_compute_non_uint32_t_xyzar.rs:5:34: error: Unexpected kernel root() parameter 'z' of type 'double'
-root_compute_non_uint32_t_xyzar.rs:5:43: error: Unexpected kernel root() parameter 'ar' of type 'uchar'
+root_compute_non_uint32_t_xyzar.rs:5:15: error: Parameter 'x' must be of type 'unsigned int'. It is of type 'int'
+root_compute_non_uint32_t_xyzar.rs:5:24: error: Parameter 'y' must be of type 'unsigned int'. It is of type 'float'
+root_compute_non_uint32_t_xyzar.rs:5:34: error: In compute kernel root(), parameter 'z' cannot appear after the 'x' and 'y' parameters
+root_compute_non_uint32_t_xyzar.rs:5:43: error: In compute kernel root(), parameter 'ar' cannot appear after the 'x' and 'y' parameters
diff --git a/tests/F_root_compute_really_bad/root_compute_really_bad.rs b/tests/F_root_compute_really_bad/root_compute_really_bad.rs
index f7bd576..1ca10d5 100644
--- a/tests/F_root_compute_really_bad/root_compute_really_bad.rs
+++ b/tests/F_root_compute_really_bad/root_compute_really_bad.rs
@@ -6,3 +6,13 @@ int root(int ain, int aout, int usrData, float x1, double y1,
          uint32_t extra1, uint32_t extra2) {
     return 10;
 }
+
+void root2(int* ain, const int* aout, int* usrData, const float* x1,
+           const double* y1, uint32_t y) {
+}
+
+int __attribute__((kernel))
+    root_kernel(int ain, int* aout, int usrData, float x1, double y1,
+                uint32_t y, uint32_t x, uint32_t extra1, uint32_t extra2) {
+  return 10;
+}
diff --git a/tests/F_root_compute_really_bad/stderr.txt.expect b/tests/F_root_compute_really_bad/stderr.txt.expect
index 458f529..7be0eb2 100644
--- a/tests/F_root_compute_really_bad/stderr.txt.expect
+++ b/tests/F_root_compute_really_bad/stderr.txt.expect
@@ -1,10 +1,21 @@
 root_compute_really_bad.rs:4:5: error: Compute kernel root() is required to return a void type
+root_compute_really_bad.rs:5:31: error: In compute kernel root(), parameter 'x' should be defined before parameter 'y'
+root_compute_really_bad.rs:6:19: error: In compute kernel root(), parameter 'extra1' cannot appear after the 'x' and 'y' parameters
+root_compute_really_bad.rs:6:36: error: In compute kernel root(), parameter 'extra2' cannot appear after the 'x' and 'y' parameters
+root_compute_really_bad.rs:4:14: error: Compute kernel root() cannot have non-pointer parameters besides 'x' and 'y'. Parameter 'ain' is of type: 'int'
+root_compute_really_bad.rs:4:23: error: Compute kernel root() cannot have non-pointer parameters besides 'x' and 'y'. Parameter 'aout' is of type: 'int'
+root_compute_really_bad.rs:4:33: error: Compute kernel root() cannot have non-pointer parameters besides 'x' and 'y'. Parameter 'usrData' is of type: 'int'
+root_compute_really_bad.rs:4:48: error: Compute kernel root() cannot have non-pointer parameters besides 'x' and 'y'. Parameter 'x1' is of type: 'float'
+root_compute_really_bad.rs:4:59: error: Compute kernel root() cannot have non-pointer parameters besides 'x' and 'y'. Parameter 'y1' is of type: 'double'
 root_compute_really_bad.rs:4:5: error: Compute kernel root() must have at least one parameter for in or out
-root_compute_really_bad.rs:4:14: error: Unexpected kernel root() parameter 'ain' of type 'int'
-root_compute_really_bad.rs:4:23: error: Unexpected kernel root() parameter 'aout' of type 'int'
-root_compute_really_bad.rs:4:33: error: Unexpected kernel root() parameter 'usrData' of type 'int'
-root_compute_really_bad.rs:4:48: error: Unexpected kernel root() parameter 'x1' of type 'float'
-root_compute_really_bad.rs:4:59: error: Unexpected kernel root() parameter 'y1' of type 'double'
-root_compute_really_bad.rs:5:31: error: Duplicate parameter entry (by position/name): 'x'
-root_compute_really_bad.rs:6:19: error: Unexpected kernel root() parameter 'extra1' of type 'uint32_t'
-root_compute_really_bad.rs:6:36: error: Unexpected kernel root() parameter 'extra2' of type 'uint32_t'
+root_compute_really_bad.rs:10:44: error: Compute kernel root2() can only have one non-const pointer parameter. Parameters 'ain' and 'usrData' are both non-const.
+root_compute_really_bad.rs:10:66: error: Unexpected parameter 'x1' for compute kernel root2()
+root_compute_really_bad.rs:11:26: error: Unexpected parameter 'y1' for compute kernel root2()
+root_compute_really_bad.rs:16:38: error: In compute kernel root_kernel(), parameter 'x' should be defined before parameter 'y'
+root_compute_really_bad.rs:16:50: error: In compute kernel root_kernel(), parameter 'extra1' cannot appear after the 'x' and 'y' parameters
+root_compute_really_bad.rs:16:67: error: In compute kernel root_kernel(), parameter 'extra2' cannot appear after the 'x' and 'y' parameters
+root_compute_really_bad.rs:15:31: error: Unrecognized parameter 'aout'. Compute kernel root_kernel() can only have one input parameter, 'x', and 'y'
+root_compute_really_bad.rs:15:31: error: Compute kernel root_kernel() cannot have parameter 'aout' of pointer type: 'int *'
+root_compute_really_bad.rs:15:41: error: Unrecognized parameter 'usrData'. Compute kernel root_kernel() can only have one input parameter, 'x', and 'y'
+root_compute_really_bad.rs:15:56: error: Unrecognized parameter 'x1'. Compute kernel root_kernel() can only have one input parameter, 'x', and 'y'
+root_compute_really_bad.rs:15:67: error: Unrecognized parameter 'y1'. Compute kernel root_kernel() can only have one input parameter, 'x', and 'y'
diff --git a/tests/F_root_compute_too_many_args/stderr.txt.expect b/tests/F_root_compute_too_many_args/stderr.txt.expect
index dc393be..56d081b 100644
--- a/tests/F_root_compute_too_many_args/stderr.txt.expect
+++ b/tests/F_root_compute_too_many_args/stderr.txt.expect
@@ -1,2 +1,2 @@
-root_compute_too_many_args.rs:6:20: error: Unexpected kernel root() parameter 'extra1' of type 'uint32_t'
-root_compute_too_many_args.rs:6:37: error: Unexpected kernel root() parameter 'extra2' of type 'uint32_t'
+root_compute_too_many_args.rs:6:20: error: In compute kernel root(), parameter 'extra1' cannot appear after the 'x' and 'y' parameters
+root_compute_too_many_args.rs:6:37: error: In compute kernel root(), parameter 'extra2' cannot appear after the 'x' and 'y' parameters