scott.linder added inline comments.
================ Comment at: llvm/include/llvm/Support/CodeGen.h:57 + /// Code generation optimization level. + enum Level : IDType { + None = 0, ///< -O0 ---------------- barannikov88 wrote: > arsenm wrote: > > scott.linder wrote: > > > This is ABI breaking, so maybe we don't want/need to define the > > > underlying type? > > This isn't in the C API, and this isn't for a release branch so I don't > > think it matters > Why did you need to restrict the underlying type? > There is no strict need, it just: * Avoids potential UB (casting an out-of-range value to an enumeration type //without a fixed underlying type// is undefined); in practice there is plenty of UB in LLVM, and a bug here wouldn't be a particularly pernicious kind of UB anyway. * Means users of the type might pack better (and we won't run out of 255 opt levels anytime soon). I originally intended to change this to an `enum class` rather than a `class` in a `namespace`, but that is slightly more disruptive and should probably be done for every other enumeration defined here at the same time. ================ Comment at: llvm/include/llvm/Support/CodeGen.h:67 + inline std::optional<Level> getLevel(IDType ID) { + if (ID < 0 || ID > 3) + return std::nullopt; ---------------- 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. 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