[PATCH] D52835: [Diagnostics] Check integer to floating point number implicit conversions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:110
+/// are usually useless
+static unsigned AdjustPrecision(unsigned precision) {
+  return (precision * 59 + 195) / 196;
----------------
Hmm.... I don't terribly understand how this function works.  Also, comment above needs to end in a period.  

Can you elaborate further as to how this works?  Those are 3 pretty suspect looking magic numbers...


================
Comment at: lib/Sema/SemaChecking.cpp:10862
+  if (Source->isIntegerType() && Target->isFloatingType()) {
+    const llvm::fltSemantics *FloatSem = nullptr;
+    if (Target->isSpecificBuiltinType(BuiltinType::Float)) {
----------------
Since there is only 1, and fltSemantics is really small (2 shorts + 2 uints), making this a 'by copy' is likely a better solution than a pointer.


================
Comment at: lib/Sema/SemaChecking.cpp:10867
+      FloatSem = &llvm::APFloat::IEEEdouble();
+    }
+
----------------
This else case ends up needing to be handled above, but why not also handle the other floating types?  


================
Comment at: lib/Sema/SemaChecking.cpp:10873
+      if (E->EvaluateAsInt(IntValue, S.Context, Expr::SE_AllowSideEffects)) {
+        if (S.SourceMgr.isInSystemMacro(CC))
+          return;
----------------
It seems like this should be checked way before we convert it to an integer or do the other float-semantics disambiguation.


================
Comment at: lib/Sema/SemaChecking.cpp:10877
+        if (FloatValue.convertFromAPInt(IntValue, Source->isSignedIntegerType(),
+                                        llvm::APFloat::rmTowardZero) !=
+            llvm::APFloat::opOK) {
----------------
Is this the rounding mode that we use?  Also, you might need to check this against some sort of current state for rounding mode.  I know that there is an effort to do FENV_ACCESS, which this might change, right?


================
Comment at: lib/Sema/SemaChecking.cpp:10883
+          precision = AdjustPrecision(precision);
+          FloatValue.toString(PrettyTargetValue, precision);
+          IntValue.toString(PrettySourceValue);
----------------
I wonder if this call (finding the precision, 'adjusting' it, then writing to a smallstring might be a better thing to pull into its own function rather than AdjustPrecision.


https://reviews.llvm.org/D52835