[PATCH] D129881: [C] Strengthen -Wint-conversion to default to an error

[Aaron Ballman via Phabricator via cfe-commits]

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