Deprecating llvm::Optional<X>::hasValue/getValue/getValueOr

LLVM Project
1

I’d like to propose deprecation of llvm::Optional<X>::hasValue/getValue/getValueOr in favor of llvm::Optional<X>::has_value/value/value_or. I am wondering if we could discuss the best way to go about it or whether we should do this in the first place.

Background

I’ve recently introduced llvm::Optional<X>::has_value/value/value_or to provide an interface similar to std::optional<X>. This has made the original methods llvm::Optional<X>::hasValue/getValue/getValueOr redundant.

Issues

  • Different people seem to have different opinions regarding whether X.hasValue() should be converted to X when it is contextually convertible to bool. One feedback I’ve received is that given instance X of llvm::Optional<bool>, X and *X are very confusing. The same applies to integers and pointers. Note that Flang’s style guide prefers X to X.has_value() in the predicate expressions of control flow statements, but I am not aware of other style guides, subproject-specific or LLVM-wide.
  • Different people seem to have different opinions regarding whether X.getValue() should be converted to X.value() or *X. Again, Flang’s style guide prefers *X to X.value() where the value presence test dominates *X.

Proposal

  • Mechanically convert hasValue() to has_value(), but drop .has_value() in the predicate expressions of control flow statements in flang.
  • Mechanically convert getValue() to value(), but prefer *X to X.value() in flang where the value presence test dominates *X.
  • Mechanically convert getValueOr() to value_or().
  • Once conversions are complete in our repository, deprecate hasValue/getValue/getValueOr for 6 months or so before removing them out of consideration for out-of-tree folks.

The rational here is to preserve the original style except where we have a clear guideline (i.e. flang).

Feedback is greatly appreciated.

2 Likes
2

Your proposal sounds very sensible to me!

3

Sorry, didn’t mean my suggestion to move towards op*/bool conversion to complicate things/add more work. Since there’s some contention, I think we should just go with preserving the equivalent/as-written.

As for Flang - I’d probably recommend just preserving the writing style and leave it to a separate change to do style cleanup in Flang if there’s stuff that’s violating their style guide. (also it’s problematic/a pity that Flang has a separate style guide (even if it delegates to LLVM’s style guide as a baseline) - a major goal of LLVM and its style guide is to make it easy for developers to move around between subprojects due to consistent style)

1 Like
4

@dblaikie, no worries. It looks like we have a path forward to move away from llvm::Optional<X>::hasValue/getValue.

At this point, Flang doesn’t seem to require many changes to move away from the old methods. I think I’ll replace X.getValue() with *X, where the presence test dominates *X. Then I’ll mechanically replace the rest with X.value().

Thanks!

1 Like
5

Do we have the ultimate goal of eliminating llvm::Optional in favor of std::optional? If not what is motivating keeping llvm::Optional now that we’ve adopted C++17?

1 Like
6

I think it’s an excellent idea for new code to use std::optional at least.

For most existing cases, the transition is just a matter of replacing Optional with std::optional.
However, to eliminate llvm::Optional, we would have to take care of a couple of details, which shouldn’t be too hard. Specifically:

  • We need to verify that all the utility functions in llvm/include/llvm/ADT/Optional.h are compatible with the standard C++ library and implement replacements. For example, raw_ostream &operator<<(raw_ostream &OS, const Optional<T> &O) is specific to LLVM.
  • We need to decide what to do with custom storage classes like MapEntryOptionalStorage, which is an implementation detail to hide the size overhead of the boolean value and ensure that sizeof(Optional<clang::DirectoryEntryRef>) == sizeof(clang::DirectoryEntryRef).

In any event, I am happy to help.

7

I would prefer sticking with our own ADT structures, even if in this case the STL implementation happens to have acceptable performance characteristics. If nothing else, std::optional cannot be forward declared (at least this is usually the case for stuff in std, please correct me if optional is an exception to that). This means that we need to include <optional> in all headers, while currently it can usually be forward-declared.

8

I prefer migrating to stl options where possible. It is less maintenance overhead. Why maintain a full-fledged implementation of optional when we can use the STL one?

9

How many files are doing that though? I could only find a handful in LLVM itself and even less in clang. Are there subprojects that are heavily relying on this?

10

Strong +1 to migrating to STL equivalents where possible (I’m assuming where performance of STL equivalents is poor, we consider it not possible here). It makes it substantially easier for users of LLVM to integrate it into other external projects, because they can use the same interfaces. Otherwise, we have to provide additional complexity to convert from STL to LLVM types and vice versa.

1 Like
11

It looks like several people suggest migration to std::optional. I’ll start submitting patches. Thanks!

12

What’s the behaviour when accessing the value of a null std::optional? For llvm::Optional I believe both operator* and value() will assert in debug/LLVM_ENABLE_ASSERTIONS builds.

13

libcxx/include/optional says:

    _LIBCPP_INLINE_VISIBILITY
    _LIBCPP_AVAILABILITY_THROW_BAD_OPTIONAL_ACCESS
    constexpr value_type const& value() const&
    {
        if (!this->has_value())
            __throw_bad_optional_access();
        return this->__get();
    }

with __throw_bad_optional_access defined as:

_LIBCPP_NORETURN
inline _LIBCPP_INLINE_VISIBILITY
_LIBCPP_AVAILABILITY_THROW_BAD_OPTIONAL_ACCESS
void __throw_bad_optional_access() {
#ifndef _LIBCPP_NO_EXCEPTIONS
        throw bad_optional_access();
#else
        _VSTD::abort();
#endif
}
14

Was this issue addressed yet? I was hoping we would have a solution to custom storage before replacing llvm::Optional with std::optional.

15

Not yet. We probably have to provide a complete template specialization.

template<>
class std::optional<MyClass> {
  :
  :
};

If you look at MapEntryOptionalStorage, it pretty much provides all important methods on its own anyway, relying on llvm::Optional only for syntax sugar like value_or, operator->, etc.

AFAICT, clang::FileEntryRef and DirectoryEntryRef are the only direct users of OptionalStorage, so duplicating much of std::optional is probably OK.

17

Hello, many patches to switch to std::optional have landed by now.

It looks like std::optional::value() isn’t available on macOS before macOS 10.13, so any call of that function breaks building for 10.12. The fix is easy (use operator* instead of value()), but that’s likely going to be a fairly annoying thing to fix going forward.

Any thoughts on what to do about that?

18

Style guide?

if (op) 
  *op

Use has_value() only in unit tests.

19

std::optional::value() has undesired exception checking semantics and calls __throw_bad_optional_access in libc++. Moreover, the API is unavailable without _LIBCPP_NO_EXCEPTIONS on older Mach-O platforms (e.g. macOS<10.13), see _LIBCPP_AVAILABILITY_BAD_OPTIONAL_ACCESS in libcxx/.

At this point, the only practical approach is to migrate value() to operator* (and sometimes operator-> or value_or). We’ll have to find previous llvm::Optional::getValue call sites like
611ffcf4e4a2ab19063174f6990969f96e9078de and change them to operator*.

My language server says there are 400+ references.

… but that’s likely going to be a fairly annoying thing to fix going forward.

Hopefully not many people use value(). Many have learned to discourage std::vector::at in favor of operator[].

I think llvm::Optional::value needs LLVM_DEPRECATED to discourage uses.

20

__throw_bad_optional_access calls abort unless exceptions are enabled so this part does not seem to be an issue.
I mean, this is not an argument to convert everything to *X and deprecate value(). The only real reason is that the API isn’t available on some older platforms.

21

I should make sure people are aware of Undo one llvm::Optional => std::optional · llvm/llvm-project@08c8280 · GitHub which we ran into with older gcc.