[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D10732: [OPENMP 4.0] Initial support for array sections.
From:       Alexey Bataev <a.bataev () hotmail ! com>
Date:       2015-07-23 10:40:41
Message-ID: da1447301b57f7f05ff707a74d2ee420 () localhost ! localdomain
[Download RAW message or body]

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




_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]