[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3245
+def warn_impcast_integer_float_precision : Warning<
+  "implicit conversion from %0 to %1 may loses integer precision">,
+  InGroup<ImplicitFloatConversion>, DefaultIgnore;
----------------
ziangwan wrote:
> nickdesaulniers wrote:
> > `may lose` or just `loses`.
> "may lose". Whether the precision loss really happens depends on the value of i (how many precision bits i has).
> 
> For example, `float e = i` where i is an integer. If i is 15 then there isn't precision loss. If i is 222222222, then there is precision loss.
This isn't done yet (reopening comment until `may loses` is respelled `may lose`).


================
Comment at: clang/lib/Sema/SemaChecking.cpp:11416
+      S.Context.getFloatTypeSemantics(QualType(TargetBT, 0)));
+    llvm::APFloat TargetFloatValue(
+      S.Context.getFloatTypeSemantics(QualType(TargetBT, 0)));
----------------
You don't need `TargetFloatValue` until later? Maybe sink it's definition below closer to where it's used.  That way we don't have to calculate this if the below condition is false.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:11430
+        SmallString<32> PrettyTargetValue;
+        TargetFloatValue.toString(PrettyTargetValue,
+          TargetPrecision);
----------------
xbolva00 wrote:
> Can you check my older patch + tests + discussion? 
> 
> I had to use other way to get this string..
> 
> 
And I don't think there's a test for this case? Or at least one that checks the printed value?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64666/new/

https://reviews.llvm.org/D64666