[PATCH] D152003: [clang] Fix `static_cast` to array of unknown bound

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/Sema/SemaType.cpp:8824
+  if (auto *Cast = dyn_cast<CXXStaticCastExpr>(E)) {
+    if (auto *SubInit = dyn_cast<CXXParenListInitExpr>(Cast->getSubExpr())) {
+      const Type *InnerType = SubInit->getType().getTypePtr();
----------------
Fznamznon wrote:
> erichkeane wrote:
> > Fznamznon wrote:
> > > erichkeane wrote:
> > > > I am not really sure this is the right way about this.  You're supposed to be testing `T`, but this looks like it is checking the type of `E`, isn't it?  I think you just need to check `Cast->getType()`.
> > > For the case I'm trying to fix, `T` is array of unknown bound, it is already checked before calling `completeExprArrayBound`. Right now when you write something like
> > > ```
> > > int (&&arr1)[1] = static_cast<int[]>(42);
> > > ```
> > > Clang actually is able to realize that parenthesized initialization is made, so it actually generates `CXXParenListInitExpr` that has type int[1]. Here I'm just paranoidally double-checking that is the case before switching the type. Maybe I don't need this double-check at all then?
> > > 
> > That makes me wonder if this is the correct place for this?  Should when we generate this type when we do the `CXXParenListInitExpr` fixup?
> > 
> > Either way, I think making this depend on that behavior (which would possibly be fragile), we should just do it based on the type passed ot the `static_cast`.
> > 
> > Another question: is this something that needs to be done for other cast types?  Does similar behavior exist for the other casts?  Should it also happen with 'clobber' casts (C-style) that are effectively static?
> > That makes me wonder if this is the correct place for this? Should when we generate this type when we do the CXXParenListInitExpr fixup?
> 
> The function called `completeExprArrayBound` seemed like an appropriate place for me.
> Could you please elaborate what you mean under `CXXParenListInitExpr fixup`? 
> 
> > is this something that needs to be done for other cast types?
> 
> I'm not sure. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1975r0.html seems to be extending only static_casts. And this was the original purpose of this bugfix.
> gcc and msvc don't agree on that matter https://godbolt.org/z/1M3ahhsYr.
>>Could you please elaborate what you mean under CXXParenListInitExpr fixup?

You mentioned that at a different place we set the type of the `CXXParenListInitExpr` to be the 1-arity array.  I was saying that if there is logic THERE based on whether its an incomplete array type it would be for the same purpose, and perhaps be the correct place to do this.

>>I'm not sure. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1975r0.html seems to be extending only static_casts. 

Yep, thats what I was hoping to see, thanks.

>>And this was the original purpose of this bugfix.

I was concerned that we increasing the inconsistency by only fixing 1 of the types of casts.  It might seem like 'feature creep', but it is very important that we make sure a patch is reasonably complete, so that it doesn't make the compiler less consistent.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152003