[PATCH] D85961: [Fixed Point] Add floating point methods to APFixedPoint.

[Bevin Hansson via Phabricator via llvm-commits]

ebevhan added inline comments.


================
Comment at: llvm/include/llvm/ADT/APFixedPoint.h:67
 
+  bool canAccommodateFloatSemantics(const fltSemantics &FloatSema) const;
+
----------------
rjmccall wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > This should have a doc comment, which should clarify that precision loss is acceptable as long as it doesn't overflow.  Also,  "accommodate" seems like the wrong direction for this: I'd expect that a fixed-point type can "accommodate" a floating-point type if the floating-point values are representable as the fixed-point type, not the reverse.  Maybe `fitsInFloatSemantics`?
> > Missed the doccomment by mistake.
> > 
> > I changed it to 'fitsInFloatSemantics'. It might be a bit misleading though, since it doesn't really check if the real value fits, but rather the value as an integer. That's why I went with something a bit more vague like "accommodate".
> Do you just want a method on FixedPointSemantics that returns the unscaled, i.e. "value as an integer" semantics?
How would we be informed of whether the semantic can fit in the floating point type, then? We'd still need a method that does that. It just seems like there'd be extra steps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85961