[PATCH] D74144: [OPENMP50]Add basic support for array-shaping operation.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> 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.

I don't think that's true; the element type of a C99 VAT can still be another VAT.  The only restriction is that we don't allow a CAT of VATs — we require the outer array to still be represented as a VAT despite have a constant immediate bound — but that's just a representational restriction and doesn't restrict what we can express.  But it doesn't matter because, as you say, we can and should use a placeholder here in order to enforce the use-restriction.



================
Comment at: clang/lib/Sema/SemaExpr.cpp:4751
+                                       LParenLoc, RParenLoc, Dims, Brackets);
+  if (!BaseTy->isAnyPointerType())
+    return ExprError(Diag(Base->getExprLoc(),
----------------
ABataev wrote:
> ABataev wrote:
> > 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.
> Forgot to mention, that I thought about possible conversion here. But it may lead to some unpredictable results, like:
> ```
> int a[3];
> ...([3][4][n])a...
> ```
> Better to do not allow this kind of operation, I think.
> 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.

The wording of the C/C++ standard is that expressions of array type decay except in certain syntactic positions.  Such expressions then *do* have pointer type for this purpose.  If precedent is to forbid array references, so be it, but that's not consistent with the normal language behavior.

> 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.

If your intent is that this only applies only to C pointers, you should just check `isPointerType()` instead of `isAnyPointerType()`, which exists for the sole purpose of also including Objective-C pointers.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4751
+                                       LParenLoc, RParenLoc, Dims, Brackets);
+  if (!BaseTy->isAnyPointerType())
+    return ExprError(Diag(Base->getExprLoc(),
----------------
rjmccall wrote:
> ABataev wrote:
> > ABataev wrote:
> > > 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.
> > Forgot to mention, that I thought about possible conversion here. But it may lead to some unpredictable results, like:
> > ```
> > int a[3];
> > ...([3][4][n])a...
> > ```
> > Better to do not allow this kind of operation, I think.
> > 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.
> 
> The wording of the C/C++ standard is that expressions of array type decay except in certain syntactic positions.  Such expressions then *do* have pointer type for this purpose.  If precedent is to forbid array references, so be it, but that's not consistent with the normal language behavior.
> 
> > 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.
> 
> If your intent is that this only applies only to C pointers, you should just check `isPointerType()` instead of `isAnyPointerType()`, which exists for the sole purpose of also including Objective-C pointers.
You can still have pointers to arrays without suppressing array decay.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4771
+    }
+    if (!Dim->isValueDependent() && !Dim->isTypeDependent()) {
+      ExprResult Result =
----------------
ABataev wrote:
> 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 :)
Type-dependence implies value-dependence anyway.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74144/new/

https://reviews.llvm.org/D74144