ABataev marked 4 inline comments as done. ABataev added a comment. In D74144#1941971 <https://reviews.llvm.org/D74144#1941971>, @rjmccall wrote:
> Okay, so these array-shaping expressions are only allowed as operands to > specific OpenMP directives? That's a plausible interpretation of "The > shape-operator can appear only in clauses where it is explicitly allowed" > from the spec <https://www.openmp.org/spec-html/5.0/openmpsu20.html>. If it > were a more general l-value expression, we could handle that just fine by > building the type using `ConstantArrayType`/`VariableArrayType` as > appropriate; but if the language intentionally restricts it, the placeholder > approach seems fine. Hi John, thanks for the review! Yes, this is not a general form of the expression, it is allowed only in a couple of clauses. We cannot build an array type since this expression does not represent a supported array type. Sizes may be non-constant anywhere in the operation, variable array type does not support it. It requires extending of the variable array types but actually it does not worth a try, since this type is not needed at all. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:4748 + Base->isInstantiationDependent() || + Base->containsUnexpandedParameterPack())) + return OMPArrayShapingExpr::Create(Context, Context.DependentTy, Base, ---------------- rjmccall wrote: > Just check `isTypeDependent()`, none of these other conditions should > interfere with type-checking. Ok, will do. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:4751 + LParenLoc, RParenLoc, Dims, Brackets); + if (!BaseTy->isAnyPointerType()) + return ExprError(Diag(Base->getExprLoc(), ---------------- rjmccall wrote: > I think you should perform DefaultFunctionArrayLvalueConversion here so that > e.g. arrays will decay to pointers, you load pointers from l-values, and so > on. If you do so, it'll handle placeholders for you. > > Do you really want to allow this to operate on non-C pointer types? 1. Standard clearly states that the type of the base expression must be a pointer. I don't think that we should perform implicit type casting here, like decay to pointers, etc. 2. It is just a simple form of checking that this is a pointer type. Since this expression is not allowed in other languages (and I filter it out at the parsing stage), I think it is ok to use the generic form of type checking. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:4758 + for (Expr *Dim : Dims) { + if (Dim->getType()->isNonOverloadPlaceholderType()) { + ExprResult Result = CheckPlaceholderExpr(Dim); ---------------- rjmccall wrote: > I think overload placeholders need to be resolved here, too. You may have > copied this code from some different place that has the ability to resolve > overloads later, but in this case that's not true. No, YouCompleteMe suggested the wrong function and I just missed it. Will fix it, thanks! ================ Comment at: clang/lib/Sema/SemaExpr.cpp:4771 + } + if (!Dim->isValueDependent() && !Dim->isTypeDependent()) { + ExprResult Result = ---------------- rjmccall wrote: > You don't really care about value-dependence here, just type-dependence. You > can check value-dependence before doing the constant-evaluation check below. Yes, just a double check to be realy-really sure :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74144/new/ https://reviews.llvm.org/D74144 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
