ilya-biryukov added a comment. In D146426#4207118 <https://reviews.llvm.org/D146426#4207118>, @aaron.ballman wrote:
> This feels like it's heading in the wrong direction -- the AST should not > have holes in it. An invalid type should be replaced by a valid type (after > diagnosing the invalid type, of course) so that we can keep as much of the > AST around as possible (for example, we typically stub in `int` and continue > compilation, as in: https://godbolt.org/z/MvxjGovGh), which should then > result in a non-null `ParmVarDecl`. This way, we don't need to sprinkle > nullptr checks all over the compiler when inspecting a function's parameters. I actually prototyped doing this and it caused extra errors with no locations and diagnostic errors. Moreover, this happens not only with `__fp16`, but for all cases when `Declarator::setInvalidType` is called (and that's quite a lot of cases). We also have checks for `nullptr` inside parameter lists in many places of the codebase, e.g. see a usage in SemaTemplateInstantiateDecl.cpp <https://github.com/llvm/llvm-project/blob/f67b481098cc30567d0f50a2b21f8f57b92052bd/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L4425>. I fully agree that the direction taken is not very good, but it has not been chosen by this patch. A more involved refactoring that would ensure parameter lists never have nulls is definitely very welcome, this change intends to quickly fix a crash we're experiencing. In D146426#4207146 <https://reviews.llvm.org/D146426#4207146>, @erichkeane wrote: > In D146426#4207118 <https://reviews.llvm.org/D146426#4207118>, @aaron.ballman > wrote: > >> This feels like it's heading in the wrong direction -- the AST should not >> have holes in it. An invalid type should be replaced by a valid type (after >> diagnosing the invalid type, of course) so that we can keep as much of the >> AST around as possible (for example, we typically stub in `int` and continue >> compilation, as in: https://godbolt.org/z/MvxjGovGh), which should then >> result in a non-null `ParmVarDecl`. This way, we don't need to sprinkle >> nullptr checks all over the compiler when inspecting a function's parameters. > > Frankly, I would like to see us just insert the `__fp16` type instead. We've > already diagnosed, so we won't go to codegen, which is where the parameter > issue is going to cause a problem. I can't imagine we have ANY code that > depends on `__fp16` and would break if it is a parameter. +1 to that, `__fp16` is a better choice here, e.g. that's something I would expect to see in AST dumps. We just need a fix that avoids `nulls` inside parameter lists. Note that the types are already handled properly, but for some reason that I didn't dig out we choose to defer setting the parameter decls until a call to `GetTypeSourceInfoForDeclarator`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146426/new/ https://reviews.llvm.org/D146426 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
