[PATCH] D139683: [APInt] provide a safe API for zext value and sext value.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon accepted this revision.
RKSimon added a comment.
This revision is now accepted and ready to land.

LGTM with a couple of minors - cheers!



================
Comment at: llvm/include/llvm/ADT/APInt.h:1501
+    return (getActiveBits() <= 64) ? std::optional<uint64_t>(getZExtValue())
+                                   : std::optional<uint64_t>();
+  };
----------------
tschuett wrote:
> RKSimon wrote:
> > tschuett wrote:
> > > `std::nullopt`?
> > +1
> `std::optional<uint64_t>(std::nullopt)`.  --> `std::nullopt` .
> 
> `std::optional<uint64_t>(getZExtValue())` ->. `getZExtValue()`.
> 
Any reason you can't remove the std::optional<uint64_t>() cast?


================
Comment at: llvm/include/llvm/ADT/APInt.h:1522
+  std::optional<int64_t> trySExtValue() const {
+    return (getActiveBits() <= 64) ? std::optional<int64_t>(getSExtValue())
+                                   : std::optional<int64_t>();
----------------
Peter wrote:
> RKSimon wrote:
> > RKSimon wrote:
> > > RKSimon wrote:
> > > > getSignificantBits - you might need to alter the unit test as 'all bits' representations will safely truncate to -1
> > > getActiveBits ->getSignificantBits 
> > Still needs to be converted to getSignificantBits to match getSExtValue
> Sry I forgot about it. This patch should be good to go.
Any reason you can't remove the std::optional<int64_t>() cast?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139683