The last RFC moved to GCC 7.1. So you have a pretty new GCC.
“Older” in the sense of “older than most people use” but yes, still officially supported (gcc 7.5 is from 2017).
Sorry. It is totally valid to run gcc 7.5 against LLVM and complain about errors. Pretty new and older can be interpreted against the minimal requirements or the current year.
Someone might want to look into Recent Optional commit(s) + clang-11 on ubuntu 20.04 cause broken compiler, many test failures · Issue #59622 · llvm/llvm-project · GitHub
I don’t think we can use operator* here, as there is nothing guaranteeing it asserts.
I’m sure I’m not the only developer who has historically used llvm::Optional::operator* without an additional assert because it was defined to do the assert internally. This implies that just swapping in std::optional for llvm::Optional is equivalent to deleting a lot of important asserts.
As an example of a change making this mistake see 467432899bc2f71842ed1b24d24c094da02af7d4 “MemoryLocation: convert Optional to std::optional”. Before that change, this code would always assert in get if it saw None:
static MemoryLocation get(const Instruction *Inst) {
return *MemoryLocation::getOrNone(Inst);
// ^ Guaranteed to assert(hasValue())
}
static Optional<MemoryLocation> getOrNone(const Instruction *Inst);
The change should have retained the assert one way or another. Assuming the value method is an issue on some platforms, the most straightforward option is to add a temporary and use a plain assert:
static MemoryLocation get(const Instruction *Inst) {
auto ML = MemoryLocation::getOrNone(Inst);
assert(ML);
return *ML;
}
static std::optional<MemoryLocation> getOrNone(const Instruction *Inst);
I think this harms readability on several fronts (stretches vertically, breaks up expressions needlessly, and forces the reader to consider the possibility that the temporary variable has some other purpose later in the scope), so I want to propose something I think is an improvement over the original llvm::Optional version: a Rust-like unwrap/expect, but implemented as a free-function. Then the above becomes:
static MemoryLocation get(const Instruction *Inst) {
return unwrap(MemoryLocation::getOrNone(Inst));
}
static std::optional<MemoryLocation> getOrNone(const Instruction *Inst);
Bikeshedding on naming is welcome, but the point is just to have an assert-and-then-dereference style function that can be used uniformly for these cases. It could be as simple as a new header llvm/ADT/Unwrap.h with a single function:
namespace llvm {
/// Assert the truthiness of, and dereference, fallible
/// wrapper-like object \p C.
template <typename T>
inline auto unwrap(T &&Wrapper) -> decltype(*std::forward<T>(Wrapper)) {
assert(Wrapper);
return *std::forward<T>(Wrapper);
}
} // namespace llvm
For cases trivially dominated by a check we could still safely allow the bare dereference operator:
std::optional<int> I;
if (I)
return *I; // Fine
Thoughts?
I’m pretty sure all standard library vendors do assert that it contains a value (at least in debug builds), but I can’t disagree that relying on that behavior is a bad idea. Every access to optional must be guarded by a check that it contains a value.
I think this harms readability on several fronts (stretches vertically, breaks up expressions needlessly, and forces the reader to consider the possibility that the temporary variable has some other purpose later in the scope)
I disagree. The assert (that should contain a message) tells the reader that there is a contract, and not that someone forgot to add a check.
a Rust-like
unwrap/expect, but implemented as a free-function.
Introducing an asserting wrapper may sound like a good idea. However, if you find yourself using it often, this probably indicates a problem with your code. Likely, you don’t need optional if you don’t check that it contains a value (i.e. if it is not really optional).
I’m pretty sure all standard library vendors do assert that it contains a value (at least in debug builds), but I can’t disagree that relying on that behavior is a bad idea. Every access to optional must be guarded by a check that it contains a value.
Unfortunately, that doesn’t even seem to be the case. I compared using operator*, value, and an assert locally and my stdlib seems to happily let the undefined operator* return, and the process exits normally with a successful status. I’ve seen reports that the same is true of at least some versions of MSVC.
I disagree. The assert (that should contain a message) tells the reader that there is a contract, and not that someone forgot to add a check.
I agree that the intent should be explicit, I just think a new function that generically captures the intent is better than requiring an intervening assignment. I also don’t think every assert needs a message, in the same way that every line of code does not need a comment, but I would certainly be happy having a version which supports a message (Rust has unwrap for no-message and expect for message, we could do the same).
Introducing an asserting wrapper may sound like a good idea. However, if you find yourself using it often, this probably indicates a problem with your code. Likely, you don’t need
optionalif you don’t check that it contains a value (i.e. if it is not really optional).
I absolutely agree, but LLVM is a large codebase, and is constantly evolving. There are always more yaks to shave when refactoring, and when making incremental improvements it is fairly common to need to express this kind of invariant just to match an existing assumption in the code.
An example is 
D141968 [NFC] Consolidate llvm::CodeGenOpt::Level handling where most of the existing places constructing CodeGenOpt::Level involve asserts, but some handle the invalid IDs directly. In making the interface correct and idiomatic for the error-handling case it becomes needlessly more cumbersome to adapt the other cases.
1 Like
In this particular case I’d personally go with assert inside getLevel since its argument is preconditioned to be within the range. parseLevel is different, and having it return optional is the right thing.
Both libstdc++ and libcxx have assertions for this if you enable them, e.g. by defining _GLIBCXX_ASSERTIONS. Perhaps all we need to do is to tweak our cmake scripts to enable _GLIBCXX_ASSERTIONS whenever LLVM_ENABLE_ASSERTIONS is ON? I would much rather do this than write more verbose code in LLVM itself.
1 Like
IMHO, assert are the wrong approach. You have to check optionals before you access them.
if (optional)
*optional
If you want to make optionals more ergonomic to handle with Rust style get_or_default, then it is totally fine. As long as there is a real check inside and no assert.
std::expected gets these kinds of functions: ⚙ D140911 [libc++] Implement P2505R5(Monadic operations for std::expected)
They’re not the “wrong” approach. No-one is suggesting that we rely on asserts instead of checking before you access them. The asserts just give you an additional level of testing (in debug builds) in case you accidentally missed an explicit check.
My bad. I am super happy that the STL checks all my accesses to optionals and detects misbehaviour.
In user code, you have to check optionals! Thus asserts are redundant.
In this particular case I’d personally go with assert inside
getLevelsince its argument is preconditioned to be within the range.parseLevelis different, and having it returnoptionalis the right thing.
It isn’t a precondition, though; consider a generic parser for options which yields an integral value. You now have some declaration somewhere else (an Options.td file) that constrains the values, but at the point you turn that integer into a CodeGenOpt::Level you are far removed from the code which ensures the value is meaningful, hence the assert. To get rid of the assert one might teach the declarative option parser to work with enumerated types directly, but that is “another yak” in this scenario. In fact, I didn’t go prove to myself that the original asserts I was replacing in this case were valid, that is not my problem, and I don’t want it to prevent a useful refactoring. We should be removing barriers to refactoring, and making the refactored result clearer and easier to read whenever possible.
Being able to push the fact that a function is partial into the return type is nice, and I think having a composable operator which does the assert concisely when appropriate also seems nice, but as it stands we can just have more boilerplate for these cases; as you say they should’t be too frequent. The bigger issue is that changes are landing which don’t do either, and that concerns me.
In user code, you have to check optionals! Thus asserts are redundant.
I agree, but the check may be far removed, see above. Asserts are useful when the invariant isn’t enforced locally, and they make insidious bugs into obvious ones.
I also pointed to a patch that went in (apparently as NFC, I can’t find a review for it) that removes asserts without adding any other checks; the code is an intermediate step towards supporting optional MemoryLocations, and I would much prefer having the assert, even if we would not accept the change which added it today. I can guarantee you someone will be hitting a difficult-to-debug issue related to this somewhere down the line, burn hours of their time, and then add back in the assert in anger.
If we can ensure we get asserts in all of the stdlibs we support (including MSVC, MacOS, etc.) then I agree it would be OK to maintain the status-quo that a dereference will assert when assertions are enabled. Short of that, I don’t think we ought to accept an indeterminate number of assertions being silently dropped as we transition to std::optional.
To help argue my case I’ll also point to the fact that we have cast<T> and that it is used frequently without a directly dominating isa<T>; I don’t think anyone would find it acceptable to delete the assert from the definition of cast<T> (in fact it only exists to add this assert, much like an unwrap would).
⚙ D142279 [cmake] Use LLVM_ENABLE_ASSERTIONS to enable assertions in libstdc++ does this. It should be trivial to do the same for libc++. I don’t know about MSVC.
If it is not precondition, it should not be asserted. That’s my whole point. This is the same as llvm_unreachable vs report_fatal_error. We use the first when the code must not be reached due to precondition, and report_fatal_error (or another error reporting mechanism) otherwise.
If something unexpected can happen, it should be checked, not asserted that it will not happen.
I just mean to say that I don’t think it ought to be a precondition of the function, but it may be an assumption/precondition of the code which is calling it.
Certainly it could be pushed down into the function, and a doc comment could describe the precondition. I just think that is worse than admitting the function is partial in the return type itself. Nobody can ever mistakenly assume it actually handles every input, and if they are working off of assumptions they are forced to assert them at the callsite.
Also from a practical standpoint we don’t need multiple versions of every partial function in the codebase, one which reports the error to the caller and one which asserts. We should be striving to reduce the cases for which the assert is appropriate anyway, so eventually one would have no uses of the asserting version of the function.
It looks like MSVC has _CONTAINER_DEBUG_LEVEL=1:
To do this consistently, we’d need to track down all uses of operator* on all std::optionals that used to be llvm::Optional. I agree that we lose some assertion coverage, but if someone wants such assertions then having them in the implementation of the operator* would be the best (as discussed in previous replies).
Sure, I am fine with relying on the fact that every implementation of the stdlib we use has some means of adding the assertion back in. I still think an explicit unwrap/expect would be nice when the code really does want to explicitly assert, but it isn’t necessary.
I’ve pushed this patch now.