Page MenuHomePhabricator

[ADT] Add getAsOr to Optional
Needs ReviewPublic

Authored by njames93 on Sat, Aug 1, 3:23 AM.

Details

Summary

Adds a method getAsOr to Optional, This functions somewhat like getValueOr however it casts the value to a different type.
This is particularily useful if the type contained in the optional is expensive to construct.

Typical use cases would be for Optional<std::string>, It would often only be needed to access the value as a StringRef.
Similar approach can be made for ArrayRef.

This function is enabled for any type that can be statically casted from the type contained in the Optional

Diff Detail

Unit TestsFailed

TimeTest
4,830 mslinux > libFuzzer.libFuzzer::cleanse.test
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/mnt/disks/ssd0/agent/llvm-project/compiler-rt/lib/fuzzer -m64 /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/fuzzer/CleanseTest.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/cleanse.test.tmp-CleanseTest

Event Timeline

njames93 created this revision.Sat, Aug 1, 3:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Aug 1, 3:23 AM
njames93 requested review of this revision.Sat, Aug 1, 3:23 AM

I forget: Have we already decided that Optional won't be replaced by std::optional? If not, then would be good to not add API surface area that doesn't match std::optional (instead preferring non-member functions, for instance).

If that's already been decided in some previous reviews, etc, this sounds OK I guess. Probably doesn't need the enable_if, I'd have thought (could just let the implementation fail as usual - there aren't competing member functions, etc) & the parameter should be U&& to use with std::forward, avoid unnecessary copies, etc.

njames93 updated this revision to Diff 282548.Mon, Aug 3, 2:42 AM

Remove call to forward as it serves no purpose for the use case

I forget: Have we already decided that Optional won't be replaced by std::optional? If not, then would be good to not add API surface area that doesn't match std::optional (instead preferring non-member functions, for instance).
If that's already been decided in some previous reviews, etc, this sounds OK I guess.

If that's the case then I'd agree with that.
But I kind of gathered it wouldn't be the case considering how much the llvm::Optional API already differs from std::optional.

Probably doesn't need the enable_if, I'd have thought (could just let the implementation fail as usual - there aren't competing member functions, etc)

Fair enough point there.

the parameter should be U&& to use with std::forward, avoid unnecessary copies, etc.

I'm inclined to disagree with that point, this is mean't to be used with types that are cheap to construct and usually passed by value.
Probably don't need the call to std::forward though.

njames93 updated this revision to Diff 282558.Mon, Aug 3, 2:58 AM

Delete the rvalue reference version as it can create dangling references

I forget: Have we already decided that Optional won't be replaced by std::optional? If not, then would be good to not add API surface area that doesn't match std::optional (instead preferring non-member functions, for instance).
If that's already been decided in some previous reviews, etc, this sounds OK I guess.

If that's the case then I'd agree with that.
But I kind of gathered it wouldn't be the case considering how much the llvm::Optional API already differs from std::optional.

Hmm, I'm not so sure - getValueOr, for instance, is in std::optional. (value_or)

You're right that 'map', for instance, is not in std::optional, though. That and maybe the "create" function are the non-std::optional API?

I think if this new function was a non-member, it'd probably tidier - wouldn't need an rvalue and non-rvalue overload, as it could use std::forward on both arguments.

llvm/include/llvm/ADT/Optional.h
295–297

I have reservations about this - for the same reason that StringRef, for instance, doesn't block being built from an rvalue (because you might (& in StringRef's case, do quite often) create one from a temporary only for use within a single statement).

Matching the sort of overload/semantics of getValueOr seems appropriate here.

Is there a particular example you have in mind where this would be a problem?

Hmm, I'm not so sure - getValueOr, for instance, is in std::optional. (value_or)

You're right that 'map', for instance, is not in std::optional, though. That and maybe the "create" function are the non-std::optional API?

And getPointer

I think if this new function was a non-member, it'd probably tidier - wouldn't need an rvalue and non-rvalue overload, as it could use std::forward on both arguments.

non-member could work I guess, and could be adapted for other cases, std::optional or pointers spring to mind.

llvm/include/llvm/ADT/Optional.h
295–297

Fair enough, I'll remove this. Though is it wise to use static_cast<U>(getValue()) or static_cast<U>(std::move(getValue()))

Hmm, I'm not so sure - getValueOr, for instance, is in std::optional. (value_or)

You're right that 'map', for instance, is not in std::optional, though. That and maybe the "create" function are the non-std::optional API?

And getPointer

Ah, right you are - I was thinking std::optional had "get()" like std::unique_ptr, but I must've got them jumbled up.

I think if this new function was a non-member, it'd probably tidier - wouldn't need an rvalue and non-rvalue overload, as it could use std::forward on both arguments.

non-member could work I guess, and could be adapted for other cases, std::optional or pointers spring to mind.

Yep. (also, wouldn't mind if the test was a bit more narrow - if possible, not using non-trivial standard library types - using explicitly written types (not necessarily written just for the test - there are other common utility/stub/mock types in this file and other test files that can be generalized a bit and reused, etc) to test certain features - narrows the scope a bit so the test isn't dependent on too much else)