scott.linder added inline comments.
================
Comment at: llvm/include/llvm/Support/CodeGen.h:67
+ inline std::optional<Level> getLevel(IDType ID) {
+ if (ID < 0 || ID > 3)
+ return std::nullopt;
----------------
mstorsjo wrote:
> scott.linder wrote:
> > barannikov88 wrote:
> > > As I can see, clients do not check for nullopt. Either add checks or
> > > replace this check with an assertion and drop std::optional (in this case
> > > `parseLevel` should be updated accordingly).
> > >
> > >
> > Good catch! I was working off of the old behavior of `llvm::Optional` and
> > assuming the new `std::optional` was guaranteed to `assert` on dereference
> > as well. I think the right interface is to expose the `optional`, so for
> > the callsites which currently do the equivalent of asserting I will just
> > carry over an explicit assert to the new version.
> >
> > I posted to
> > https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716/26
> > to discuss the issue more generally, as at least some of the patches which
> > moved code from `llvm::Optional` to `std::optional` accidentally dropped
> > assertions.
> This causes massive amounts of warnings when built with GCC:
> ```
> ../include/llvm/Support/CodeGen.h:67:12: warning: comparison is always false
> due to limited range of data type [-Wtype-limits]
> 67 | if (ID < 0 || ID > 3)
> | ~~~^~~
> ```
Fixed in 2dc33713de7ceae3e28a13be1d72c862ec0efb2c
Thank you for reporting, sorry for the noise!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141968/new/
https://reviews.llvm.org/D141968
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
