[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo 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;
----------------
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)
      |         ~~~^~~
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141968