ArcsinX marked 3 inline comments as done.
ArcsinX added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:5779
QualType ParamTy = Proto->getParamType(ArgIdx);
+ if (ParamTy->containsErrors())
+ continue;
----------------
hokein wrote:
> ArcsinX wrote:
> > hokein wrote:
> > > It looks like for the failure case the `ParamTy` for the parameter is a
> > > dependent array type, and it violates the "non-dependent" assumption of
> > > `clang::ASTContext::getTypeInfoImpl` which is called by
> > > `getTypeAlignInChars` in `CheckArgAlignment`.
> > >
> > > so I'd suggest moving the fix to `CheckArgAlignment` line 5685 (adding a
> > > `ParamTy->isDependentType()` to the `if` condition).
> > When I found this problem I fixed it like you are suggesting. But after
> > that I replaced it with this check, because it seems there is no reason to
> > try to check something on code with errors.
> >
> > I mean that even if we are not crashing, results from `CheckArgAlignment()`
> > can't be trusted if we are passing something with errors to it.
> > because it seems there is no reason to try to check something on code with
> > errors.
>
> I think this is case-by-case (for this case, it may be true) -- in some cases
> (see test7 and test8 for example in `recovery-expr-type.cpp`), we do want to
> check something even on the code with errors to emit useful secondary
> diagnostics. In general we want to emit a full list of diagnostics and at the
> same time avoid any suspicious diagnostics, however achieving both goals is
> hard.
>
> Depending on how fatal the error is
>
> - if the error is fatal, RecoveryError is just a dependent-type wrapper, we
> should not call check* to emit diagnostics (we're less certain about the
> quality of these diagnostics), this is mostly done by leveraging on the
> existing template dependent mechanism;
> - if the error is minor, RecoveryError preserves a concrete type, we might
> want call check* to emit diagnostics as we're more confident;
>
> The current solution makes sense to fix the crash, but I think the main
> reason is that `CheckArgAlignment` now can be invoked with a dependent-type
> parameter in non-template context (after we extended the "dependent" concept
> from depending on template parameters to depending on template parameters and
> errors), so we should fix `CheckArgAlignment`.
>
Thanks for clarification. Moved check inside CheckArgAlignment() as you
suggested.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133886/new/
https://reviews.llvm.org/D133886
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
