hokein added a comment.
thanks, I think we can simplify a bit more.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:4543
// getTypeAlignInChars requires complete types
- if (ParamTy->isIncompleteType() || ArgTy->isIncompleteType() ||
- ParamTy->isUndeducedType() || ArgTy->isUndeducedType())
+ auto CheckTypeOK = [](QualType Ty) {
+ if (Ty->isIncompleteType() || Ty->isUndeducedType())
----------------
adamcz wrote:
> hokein wrote:
> > The current fix is on top of kadir's fix, now the code becomes more
> > add-hoc, it looks like a smell to me.
> >
> >
> > It looks like `CheckArgAlignment` has an implicit invariant (the arg type
> > should never be undeduced), and this invariant was enforced by dropping the
> > invalid AST. Now with `RecoveryExpr`, we might break the invariant -- when
> > the RecoveryExpr preserves an `undeduced` type, like the test case in this
> > patch (which is done via the
> > [code](https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExprCXX.cpp#L1420)).
> >
> >
> > I think a right fix is to fix in the `RecoveryExpr` side, `RecoveryExpr`
> > should not preserve an undeduced type, we did it for function return type
> > already (https://reviews.llvm.org/D87350), but it didn't cover all places.
> > I think a general fixing place is `Sema::CreateRecoveryExpr`, if the
> > passing `T` is undeduced, fallback to dependent type.
> >
> Ooh, interesting. Thanks for the info.
>
> I updated the change to catch this in CreateRecoveryExpr as well. I kept my
> original changes too, they shouldn't harm anything and it's better than
> crashing.
>
> Let me know what you think about this now.
> I kept my original changes too, they shouldn't harm anything and it's better
> than crashing.
I'm a bit nervous about keeping the change/extending the undeduced-type here,
this looks like a defensive programming, it might hide some bugs in the future.
As we have figured out the root cause, I'd also remove the bailout
`isUndeducedType` case here.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:19668
T = Context.DependentTy;
+ else if (const auto *U = T->getUnqualifiedDesugaredType())
+ if (U->isUndeducedType())
----------------
I'm not sure we need this well. I think the above `T->isUndeducedType()` should
be enough to fix all known crashes. I'd just remove this one.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100667/new/
https://reviews.llvm.org/D100667
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
