aaron.ballman added inline comments.
================ Comment at: clang/lib/Sema/SemaExprCXX.cpp:8483 + if ((SemaRef.getLangOpts().CPlusPlus && Trap.hasErrorOccurred()) || + Res.isInvalid()) return ExprError(); ---------------- aaron.ballman wrote: > efriedma wrote: > > I'd prefer not to introduce unnecessary differences in C vs. C++ handling > > like this... > > > > This seems weird for two reasons: > > > > - We don't use ext_typecheck_convert_pointer_int in C++, so there's not > > really any reason to mark it SFINAEFailure > > - Whatever code is calling this probably doesn't want an SFINAE context. > > Is this coming from TransformToPotentiallyEvaluated? We don't want SFINAE > > traps from there, in C or C++. > > We don't use ext_typecheck_convert_pointer_int in C++, so there's not > > really any reason to mark it SFINAEFailure > > The original changes came from: > https://github.com/llvm/llvm-project/issues/39709 but I see now that we used > to treat this as a warning in C++ rather than an error. So adding > SFINAEFailure made sense at that point, but I think you're right that it > should be removed now. I'll try that locally and see if anything explodes as > a result. > > > Whatever code is calling this probably doesn't want an SFINAE context. Is > > this coming from TransformToPotentiallyEvaluated? We don't want SFINAE > > traps from there, in C or C++. > > Typo correction went haywire. Specifically: > https://github.com/llvm/llvm-project/blob/main/clang/test/Sema/typo-correction-ambiguity.c#L13 > > We would typo correct `v_231` to be `v_2_0` here, and when deciding whether > that was valid or not, we'd hit the SFINAE trap in C code and that changed > the behavior of the test to add suggestions instead of silence them. > > I think it's incredibly weird to ever use SFINAE in C, even by accident. > I'll try that locally and see if anything explodes as a result. All my local tests passed, so I've made the change and we'll see if precommit CI finds anything I didn't. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129881/new/ https://reviews.llvm.org/D129881 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits