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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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

sorry, what I was suggesting was a /local/ revert (not upstreamed), then apply the more nuanced transformation (that preserves explicit test/access where it's written originally) on top of that, then merge those two commits and the result should be one commit that touches only the places that need to be corrected that lost the explicit access/testing.

> Also deprecating the old spelling methods seems reasonable, or just adding a comment.

I think that probably won't be necessary - I assume @kazu plans to complete this migration and remove the old methods entirely once all the uses are cleaned up. But might be necessary to use deprecation attributes for a while to help out-of-tree folks migrate off.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128131