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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
