[PATCH] D128131: [ADT] Add has_value, value, value_or to llvm::Optional

Tue Jun 28 16:12:00 PDT 2022

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

In D128131#3594425 <https://reviews.llvm.org/D128131#3594425>, @dblaikie wrote:

> Sounds good. Though it might be worth migrating/normalising uses to `op*` (rather than `value()`) and bool testing (where it do any have to be explicit) instead of has_value() when migrating callers.

As mentioned here by @DaniilSuchkov <https://reviews.llvm.org/rGd08f34b592ff06ccb1f36da88ec09aa926427a4d#1100448>, and here by me <https://reviews.llvm.org/rGd08f34b592ff06ccb1f36da88ec09aa926427a4d#1100526>, this does not always make it easier to read the code. On the contrary, this can easily lead to confusion.

As far as I can tell, there is no guideline in our coding style for these changes, as @clementval pointed out <https://reviews.llvm.org/rGaa8feeefd3ac6c78ee8f67bf033976fc7d68bc6d#1099539>. I'd suggest a discourse post or guideline patch so we can have the discussion if this is really helpful, especially as an automatically applied code change.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128131