Use std::optional to clean up some code and prevent use-after-free bugs

[abadams]

A lot of this code was pre C++-17, and could be improved using std::optional. This would have avoided #8482 and arguably the bug underlying #8469

[abadams]

clang-tidy is flagging this as an unchecked use of a std::optional:

    if (is_interleaved_ramp(mul->a, scope, result) &&
            (b = as_const_int(mul->b))) {
            result->base = simplify(result->base * (int)(*b));

... that's checked, right?

We also have it complaining about this:

           internal_assert(event);
            if (*event == halide_trace_begin_realization || *event == halide_trace_end_realization) 

because it doesn't see the assert as checking. Perhaps I should just disable that clang-tidy check.

[steven-johnson]

style nit: is_foo implies the function returns bool, which these no longer do. probably as_const_foo is now a better name for these.

[abadams]

The issue is that it doesn't return a const power of two. It returns the number of bits in it, if it is one. So "as" is actively misleading. I couldn't immediately think of a better name. If you treat the return value as a bool, "is" still makes sense. Open to other suggestions. exact_integer_log_2?

[steven-johnson]

Ah, ok then... name is fine I guess

[steven-johnson]

style q: if this preferable to returning std::nullopt? I would have thought the latter to be better, but whatever...

[abadams]

It felt natural to me because it looks like the empty set, and "nullopt" didn't seem like it communicated anything useful (there's no null pointer involved, so why "null"). But I don't feel strongly and can change it if you think it would be an improvement.

[mcourteaux]

Returning nullopt is a good habbit, as it's ambiguous for struct types that take an empty constructor. What is std::optional<std::string> test = {}? I honestly don't know... https://godbolt.org/z/G683WKTxa Godbolt shows it will take the nullopt, instead of the no-args-constructed object.

[abadams]

I don't agree it's ambiguous, because you'd have to say {{}} to get an optional empty-constructor struct. {} is always just default construction of the actual return type.

But that said, that's the third time it's given someone pause when reading it (Alex also mentioned it offline), so I'm definitely changing it to nullopt at this point.

[mcourteaux]

Personal taste: I dislike the use of auto. You lose clarity.

[steven-johnson]

Personal taste: I dislike the use of auto. You lose clarity.

It depends on the context IMHO. When it avoid repeating a type it's a clear win. When it omits the type entirely, less so -- IMHO no one should have to use an IDE to be able to easily read the code.

[abadams]

In this case I did it for two reasons:

1. It's worth trying to make if (std::optional<int64_t> x = as_const_int(e)) { a little shorter.
2. duck-typing. If code that calls as_const_int had already been written that way then I would have been able to do this refactor without touching it.