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

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

Your proposal sounds very sensible to me!

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

@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

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

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.

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.

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?

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?

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

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

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.

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
}

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

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.