[PATCH] D10732: [OPENMP 4.0] Initial support for array sections.

[Alexey Bataev]

ABataev added a comment.

Richard, thanks for the review!


================
Comment at: include/clang/AST/Expr.h:2146
@@ -2145,1 +2145,3 @@
 
+/// \brief OpenMP 4.0 [2.4, Array Sections].
+/// To specify an array section in an OpenMP construct, array subscript
----------------
rsmith wrote:
> Have you considered starting an ExprOpenMP.h for OpenMP expressions?
Ok, moved it to ExprOpenMP.h

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4915-4916
@@ +4914,4 @@
+  "section of pointer to incomplete type %0">;
+def err_section_negative : Error<
+  "section %select{lower bound|length}0 is evaluated to a negative value">;
+def err_section_length_undefined : Error<
----------------
rsmith wrote:
> "section %select{...} evaluated to negative value %1"?
Fixed

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4918
@@ +4917,3 @@
+def err_section_length_undefined : Error<
+  "section length is undefined, but subscripted value is not a sized array">;
+
----------------
rsmith wrote:
> undefined -> unspecified
> 
> sized array -> array
> 
> (Can you take a section on a VLA? There doesn't seem to be any obvious reason why not, since presumably you could construct the VLA type [old_bound - start] as the result.)
Fixed

================
Comment at: lib/Sema/SemaChecking.cpp:8469-8475
@@ -8464,2 +8468,9 @@
       }
+      case Stmt::ArraySectionExprClass: {
+        const ArraySectionExpr *ASE = cast<ArraySectionExpr>(expr);
+        if (ASE->getLowerBound())
+          CheckArrayAccess(ASE->getBase(), ASE->getLowerBound(),
+                           /**ASE=*/nullptr, AllowOnePastEnd > 0);
+        return;
+      }
       case Stmt::UnaryOperatorClass: {
----------------
rsmith wrote:
> Can you also check the end of the section here?
Ok, added

================
Comment at: lib/Sema/SemaExpr.cpp:3973-3978
@@ -3945,1 +3972,8 @@
                               Expr *idx, SourceLocation rbLoc) {
+  if (isArraySectionType(base->IgnoreImpCasts()->getType()))
+    return ActOnArraySectionExpr(S, base, lbLoc, idx, SourceLocation(),
+                                 /*Length=*/nullptr, rbLoc);
+  else if (isArraySectionType(idx->IgnoreImpCasts()->getType()))
+    return ActOnArraySectionExpr(S, idx, lbLoc, base, SourceLocation(),
+                                 /*Length=*/nullptr, rbLoc);
+
----------------
rsmith wrote:
> What should happen here:
> 
>   int x[5];
>   int y[10];
>   (b ? x : y[5:])[3]
> 
> ? Or here:
> 
>   int x[5][5];
>   (+x[2:])[2]
Here we should get error messages. Reworked it to allow array sections only in base part.

================
Comment at: lib/Sema/SemaExpr.cpp:4119
@@ +4118,3 @@
+      // OpenMP 4.0, [2.4 Array Sections]
+      // The lower-bound and length must evaluate to non-negative integers.
+      if (LowerBoundValue.isNegative()) {
----------------
rsmith wrote:
> Is this a constraint, or is this semantics? (Are these bounds required to be ICEs?)
Bounds are not required to be ICEs. But if they are ICEs we must check that all these values are non-negative


http://reviews.llvm.org/D10732