This is an archive of the discontinued LLVM Phabricator instance.

[Support] Add transformOptional
ClosedPublic

Authored by kazu on Dec 10 2022, 3:22 PM.

Details

Summary

llvm::Optional<T> has transform, which is equivalent to
std::optional<T>::transform. The problem is that
std::optional<T>::transform won't be available until C++23, implying
that we probably cannot use it in our codebase untli 2028 or so. We
certainly don't want to keep llvm::Optional just for transform.

This patch adds transformOptional to STLForwardCompat.h so that we can
use transform during the migration to std::optional and beyond.

I've shamelessly borrowed the implementation and test from
llvm/include/llvm/ADT/Optional.h and
llvm/unittests/ADT/OptionalTest.cpp, respectively.

This is part of an effort to migrate from llvm::Optional to
std::optional:

https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716

Diff Detail

Event Timeline

kazu created this revision.Dec 10 2022, 3:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2022, 3:22 PM
kazu requested review of this revision.Dec 10 2022, 3:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2022, 3:22 PM

Could we call it transform, or does that complicate things with overload resolution/other functions named transform?

llvm/unittests/ADT/STLForwardCompatTest.cpp
50–79

Do we have enough of these for various unit tests we could have a common/shared one?

kazu added a comment.Dec 10 2022, 5:56 PM

Could we call it transform, or does that complicate things with overload resolution/other functions named transform?

The compiler and overload resolutions are OK. With the function signature being very narrowly scoped, the proposed function should only match the intended use. (In fact, I used to call this function transform while working on the patch.)

I am not sure if people are OK. clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:221 uses both llvm::transform (defined in llvm/include/llvm/ADT/STLExtras.h) and llvm::Optional<T>::transform. Having two instances of llvm::transform in one statement is pretty confusing.

But then I personally find the name of transformOptional a bit ugly, which you may also be implying, so I am sitting on the fence as far as the naming goes. You get to break the tie for me. :-)

I'll look into sharing MoveOnly. Maybe I can come up with a preparation patch to share that.

kazu updated this revision to Diff 481900.Dec 11 2022, 12:53 AM

I've renamed transformOptional to transform.

I am now using MoveOnly, which has been moved to a separate file.

kazu added a comment.Dec 11 2022, 12:53 AM

PTAL. Thanks!

Having llvm::transform not doing what std::transform does appears confusing, so even if the name transformOptional may look strange, I'd vote +1 for it...
A distinct name helps future => std::optional<T>::transform migration as well although that will take many years.

Don't have strong feelings - maybe split the difference and go with transformOpt?

kazu updated this revision to Diff 482726.Dec 13 2022, 11:53 PM

Went back to the original naming -- transformOptional.

kazu added a comment.Dec 13 2022, 11:56 PM

Don't have strong feelings - maybe split the difference and go with transformOpt?

Hmm. Could we actually go with my original transformOptional? Opt could mean command line options or optimizations depending on contexts.

dblaikie accepted this revision.Dec 14 2022, 10:56 AM

Don't have strong feelings - maybe split the difference and go with transformOpt?

Hmm. Could we actually go with my original transformOptional? Opt could mean command line options or optimizations depending on contexts.

Fair enough

This revision is now accepted and ready to land.Dec 14 2022, 10:56 AM
This revision was landed with ongoing or failed builds.Dec 14 2022, 2:51 PM
This revision was automatically updated to reflect the committed changes.