mizvekov added a comment. In D104680#2839309 <https://reviews.llvm.org/D104680#2839309>, @rsmith wrote:
> I would prefer to split this into two changes: > ... > Does that sound reasonable? Yeah that is fine, totally understand ;) ================ Comment at: clang/lib/Sema/SemaExpr.cpp:11815 + if (IsError) + return Opc != BO_Cmp ? Context.getLogicalOperationType() : QualType(); + } ---------------- rsmith wrote: > mizvekov wrote: > > Quuxplusone wrote: > > > Peanut gallery says: Is `QualType()` the right "placeholder" to return > > > here? IIUC, this is the situation where we've diagnosed an ill-formed > > > expression and are just trying to do error-recovery: if the expression > > > looks like `x < y` then we assume the programmer wants it to return > > > `bool`, and if the expression looks like `x <=> y` then we assume the > > > programmer wants it to return... `QualType()`? Is that the same thing > > > we'd do for e.g. `x + y` or `undeclaredfunction(x)`? (If so, good, I > > > think.) > > So right now, this is the same we are doing for three-way comparison > > between builtin types we do not support / recognize. Check just below in > > this same function. Just look for the call to > > `getComparisonCategoryForBuiltinCmp`. > > > > I agree completely with you and I was going to raise the same point, but I > > would prefer we changed both places at once instead of fixing it just here, > > so I think this should be a follow up task: Find something more appropriate > > to return here. > A null `QualType` is in this case treated as an indication that the > comparison is invalid, recovery failed, and the callee has already produced a > diagnostic. (It's a bit sad that we don't generally use a separate > `TypeResult` for this kind of thing in order to distinguish between the "no > type" answer and the "invalid and I've produced a diagnostic" answer, but if > we wanted to change that I think we should aim to do it holistically across > all of Clang.) Hmm. My thinking here is that it would be totally appropriate to error recovery with a 'strong_ordering' type, since that can decay into all the other orderings, making it kind of a safe bet that it would not cause further problems. The idea of a separate type for error recovery is intriguing. But I wonder if it would be appropriate in some cases, but not all of them. * The cases where all the options are equally bad and we have no good guesses would fit the bill, but this is not the case here. * When we have a good safe bet, we can in principle just use that type. But maybe it would make sense to add a bit to it that means something like "but don't take this too seriously", which could be used to selectively suppress some diagnostics downstream. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104680/new/ https://reviews.llvm.org/D104680 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
