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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D128131#3617189 <https://reviews.llvm.org/D128131#3617189>, @jdoerfert wrote:

> 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.

I don't think all cleanups need style guide wording (see a lot of @kazu's other cleanups, such as migrating to range-based algorithms and other LLVM wrappers rather than iterator-pair based standard algorithms). But yeah, totally open to some discussion about this particular one.

Not sure if it needs to go on discourse, or we can have it here - I'd be OK with a more conservative cleanup of mapping "hasValue" to "has_value" and existing implicit bool checking remains the same. (& getValue -> value() and op* remains the same.

Though I personally find bool+* to be generally easier, because it's consistent with other things like pointers, so I don't have to think about which syntax to use for this optional-ish thing. (admittedly optional<bool> is particularly hairy) - but it's not worth a big discussion/trying to make that policy change, I don't think.

@kazu - sorry for the poor advice. Would it be especially hard to fixup the already refactored code to preserve the spelling-type used (eg: migrating hasValue -> has_value, etc)? (could probably do this without actually reverting all the cleanup upstream - revert it locally, then apply a modified refactoring that preserves spelling, then squash those commits and you should have a patch that only changes the places that need to be updated to the explicit spelling?)

Would that be sufficient/good @jdoerfert  ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128131