HerrCai0907 added inline comments.

================
Comment at: clang/lib/Sema/SemaType.cpp:2583
+    if (ArraySize->containsErrors()) {
+      RecoveryExpr *RE = RecoveryExpr::Create(
+          Context, ArraySize->getType(), ArraySize->getBeginLoc(),
----------------
erichkeane wrote:
> HerrCai0907 wrote:
> > erichkeane wrote:
> > > Actually thinking further... rather than create a NEW RecoveryExpr, is 
> > > there a problem KEEPING the ArraySize expression?  It'd likely give more 
> > > information/keep more AST consistency.
> > > 
> > > ALSO, creating this as size-0 has some implications we probably don't 
> > > want.  I wonder if a different for the placeholder would be better?  
> > > 
> > > I'D ALSO suggest hoisting this ArraySize->containsError out of the 
> > > else-if and into its own branch (rather than inside the dependent checks).
> > > KEEPING the ArraySize expression
> > Agree.
> > 
> > > creating this as size-0 has some implications we probably don't want.
> > Do you think we can modify it as 1? But I don't find a better way to 
> > identifier it as undef or other.
> > 
> > > hoisting this ArraySize->containsError out of the else-if 
> > Done
> I don't have good evidence for what else we could do (whether an incomplete 
> or variable type would be better), but at least 1 is 'legal', so perhaps we 
> can live with this for now and see if there is any fallout.
> 
> What happens if we try to 'merge' a valid and an invalid here? Does it make 
> the experience worse?  So same example you added below, but the 1st one has 
> no error in its brackets?
I add test case for this scenario. I think it will not confuse developer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149612

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to