hokein added a comment. In D149612#4346068 <https://reviews.llvm.org/D149612#4346068>, @sammccall wrote:
> Unless I'm wildly misremembering (@hokein knows better), the type should also > be dependent in C. > > The intuition is that the code has some meaning that depends on how the error > is resolved (by the programmer changing the code!), and that "dependent" is a > good first-approximation way to model this (defer further analysis, suppress > diagnostics) and "contains-errors" allows us to specifically handle the cases > that need it. > > Of course this allows dependent constructs to occur in new places, in > non-template code in C++ (this was handled quite early), and also in C (this > is why the "all languages" D89046 <https://reviews.llvm.org/D89046> came > later). > > I'm surprised this only came up now! Ideally I think we'd use > `DependentSizedArrayType` as the types of these variables, dependent types > are supposed to work in C. The dependent type should be able to work for C (we have extended clang to support dependence in C as `RecoveryExpr` heavily relies on clang's dependence mechanism). We're probably missing some places that need to handle dependent-type for C (like this case), we should fix it by making the C-only codepath properly support dependence. That being said, it is expected to see dependent types for C, we should not change the type in order to fix the crash. > It seems OK to have mergeTypes() return QualType() for types that have errors > in them - this will result in a followup diagnostic that ideally we'd drop, > but we also emit that diagnostic in this case in C++. You could also make the > case for returning the first type, or the type with errors, or the type > without errors (i.e. *assume* that the types are the same after errors are > fixed). The `mergeTypes` is a widely-used method (for C and C++ code paths), and the "non-dependent-type" seems like an important invariant for this method. I'm nervous about removing it, I think it'd be better to keep it. Similar to other callers, we can make the caller (here it is `MergeVarDeclTypes`) guarantee this invariant. An "easy" alternative, is to bail out if we see the `containsErrors` bit on the `Old` or `New` type in `MergeVarDeclTypes`. A bonus is that it can drop a not-quite-useful followup diagnostic (`redefinition of 'x' with a different type: 'char[((sizeof (<recovery-expr>(d))) == 8)]' vs 'char[((sizeof (<recovery-expr>(d)) == 8))]'`) for C++. 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
