rsmith added a comment. This seems like a very strange extension. Can you point me at where in the OpenMP specification it's defined?
The implementation here seems to be half treating it as producing a single value (like an implicit parameter pack) and half treating it as producing an array. Which is the intended model? What is the type of 'x[5:]' supposed to be? (Same as 'x[5]' or same as 'x' but with a smaller bound?) ================ 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 ---------------- Have you considered starting an ExprOpenMP.h for OpenMP expressions? ================ Comment at: include/clang/AST/Expr.h:2184-2185 @@ +2183,4 @@ + Base->isTypeDependent() || + (LowerBound && LowerBound->isTypeDependent()) || + (Length && Length->isTypeDependent()), + Base->isValueDependent() || ---------------- What's the type of an array section? Given 'int n[8]', is 'n[:4]' an 'int[4]'? If so, the section is also type-dependent whenever the lower bound or length is value-dependent. ================ 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< ---------------- "section %select{...} evaluated to negative value %1"? ================ 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">; + ---------------- 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.) ================ Comment at: lib/Parse/ParseExpr.cpp:1406-1409 @@ +1405,6 @@ + } else { + bool MayArraySection = ArraySectionAllowed; + ArraySectionExpressionRAIIObject RAII(*this, /*Value=*/false); + // Parse [: or [ expr or [ expr : + if (!MayArraySection || !Tok.is(tok::colon)) { + // [ expr ---------------- What are you trying to achieve here? It looks like this bans a[ : *a[4:] ] ... which seems like it should be allowed, given that `a[4:]` is supposed to still have array type. ================ Comment at: lib/Parse/ParseOpenMP.cpp:767 @@ -766,2 +766,3 @@ Tok.isNot(tok::annot_pragma_openmp_end))) { - ColonProtectionRAIIObject ColonRAII(*this, MayHaveTail); + ArraySectionExpressionRAIIObject RAII(*this, Kind == OMPC_depend); + ColonProtectionRAIIObject ColonRAII(*this, ---------------- What's this for? ================ Comment at: lib/Parse/RAIIObjectsForParser.h:291-292 @@ +290,4 @@ + /// \brief This sets the Parser::ArraySectionAllowed bool and + /// restores it when destroyed. It it also manages Parser::ColonIsSacred for + /// correct parsing of array sections. + class ArraySectionExpressionRAIIObject { ---------------- It does not appear to manage `ColonIsSacred`... ================ 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: { ---------------- Can you also check the end of the section here? ================ Comment at: lib/Sema/SemaExpr.cpp:510-519 @@ +509,12 @@ + SourceRange Brackets) { + if (isArraySectionType(OriginalTy)) { + auto *VAT = C.getAsVariableArrayType(OriginalTy); + return C.getVariableArrayType( + createArraySectionType(C, VAT->getElementType(), ResultTy, Size, + Brackets), + VAT->getSizeExpr(), VAT->getSizeModifier(), + VAT->getIndexTypeCVRQualifiers(), VAT->getBracketsRange()); + } + return C.getVariableArrayType(ResultTy, Size, ArrayType::ArraySection, + /*IndexTypeQuals=*/0, Brackets); +} ---------------- Should we instead create a ConstantArrayType if the size is a constant expression? ================ 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); + ---------------- What should happen here: int x[5]; int y[10]; (b ? x : y[5:])[3] ? Or here: int x[5][5]; (+x[2:])[2] ================ Comment at: lib/Sema/SemaExpr.cpp:4116-4117 @@ +4115,4 @@ + llvm::APSInt LowerBoundValue; + if (LowerBound->EvaluateAsInt(LowerBoundValue, Context, + Expr::SE_AllowSideEffects)) { + // OpenMP 4.0, [2.4 Array Sections] ---------------- Whether we accept a program should not depend on the foibles of our constant folding -- you should only reject if the value is an ICE. Warning in the non-ICE case is OK, though. ================ 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()) { ---------------- Is this a constraint, or is this semantics? (Are these bounds required to be ICEs?) http://reviews.llvm.org/D10732 _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits