[PATCH] D149612: [Sema] avoid merge error type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: sammccall, hokein.
aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaType.cpp:2582
   } else if (ArraySize->isTypeDependent() || ArraySize->isValueDependent()) {
-    T = Context.getDependentSizedArrayType(T, ArraySize, ASM, Quals, Brackets);
+    if (getLangOpts().CPlusPlus) {
+      T = Context.getDependentSizedArrayType(T, ArraySize, ASM, Quals,
----------------
erichkeane wrote:
> HerrCai0907 wrote:
> > erichkeane wrote:
> > > So I'm still not sure this is the 'right' away about it.  I think we should instead properly handle the `containsErrors` case and just always create a non-dependent sized array, except with the `RecoveryExpr` having the correct type.
> > For CPP, dependent sized array is acceptable and necessary.
> > 
> > For C, I don't think SemaType is a correctly way to resolve TypoExpr, It should be done by `ActOnXXX`.
> Why is the dependent array necessary for C++?  I also don't get your C logic here... I think that Nullptr should be a RecoveryExpr of the proper type.
> Why is the dependent array necessary for C++?

It's how we handle error recovery. I agree with @HerrCai0907 that we should keep the dependent-sized array in C++.

To me, the real question here is: what happens when a recovery expression is needed in order to form a type? Should we even *get* a recovery expression in this case? (And should the answer be different between C and C++?)

CC @hokein and @sammccall who were active on https://reviews.llvm.org/D89046 which introduced this issue. I'm not certain what design y'all were thinking of for this sort of situation.


================
Comment at: clang/test/AST/ast-dump-types-errors.cpp:5
   using ContainsErrors = int[sizeof(undef())];
-  // CHECK: DependentSizedArrayType {{.*}} contains-errors dependent
+  // CHECK: ConstantArrayType {{.*}} 'int[1]' contains-errors dependent 1
 }
----------------
I don't think we should be changing the behavior in C++, this is a C-specific issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149612