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

Fri Jul 1 11:44:33 PDT 2022

jdoerfert added a comment.

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

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

There is two problems with this approach (no style guide wording and no "announcement"):

1. People might have reasonable arguments against and no chance to bring them up.
2. People do not realize what happened and will continue to introduce the "old" style which somewhat defeats the purpose. Playing devils advocate, people might just undo the style changes with the same argument they were introduced (no need for discussion or style guide changes).

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

I'm totally fine with `hasValue` -> `has_value` (and similar) to phase out llvm::Optional (if that is the actual plan). We can also "encourage people" to use these by deprecating the old ones.

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

It's not only optional<bool>, int, ptr, can also be confusing.
The example I flagged in on of the patches changed:

  `if (... && Size1.hasValue() && ...)` to `if (... && Size1 && ...)`. 

I am very certain most people will read the former as "Size1 has a value" and the latter as "Size1 is not null". Which is arguably not an improvement.

> @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  ?

I don't think we should revert everything, that is causing churn. We should read through and flag the few problematic situations though which we move to `std::Optional` spelling. Also deprecating the old spelling methods seems reasonable, or just adding a comment.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128131