read_only tests insufficient

[brevzin]

I thought I'd open an issue. Per twitter, this test: https://github.com/tcbrindle/flux/blob/b423b21d37f510f7793b6f1bf4e6e6f187fc9a4d/test/test_read_only.cpp#L18-L35

doesn't actually test anything about read_only, since flux::ref(arr) is already a read-only sequence, so flux::read_only(flux::ref(arr)) (correctly!) just gives you back flux::ref(arr). You want flux::mut_ref(arr) there.

This also begs the question of to what extend this is a valid implementation of read_only:

 struct read_only_fn {
     template <adaptable_sequence Seq>
     [[nodiscard]]
     constexpr auto operator()(Seq&& seq) const -> read_only_sequence auto
     {
         if constexpr (read_only_sequence<Seq>) {
             return FLUX_FWD(seq);
         } else {
-            return read_only_adaptor<std::decay_t<Seq>>(FLUX_FWD(seq));
+            return flux::map(FLUX_FWD(seq), [](auto&& elem) -> const_element_t<Seq> {
+                return FLUX_FWD(elem);
+            });
         }
     }
 };

It's very nearly correct, the only thing that should break is the contiguous_sequence check in the test. The fact that read_only is a map, just one that's potentially contiguous, feels like it should be implemented like this:

template <class T>
struct cast_to_const {
    template <class U>
    constexpr auto operator()(U&& rhs) const -> T {
        return FLUX_FWD(rhs);
    }
};

template <sequence Base>
    requires (not read_only_sequence<Base>)
struct read_only_adaptor : map_adaptor<Base, cast_to_const<const_element_t<Base>>>
{
    using map = map_adaptor<Base, cast_to_const<const_element_t<Base>>>;

public:
    constexpr read_only_adaptor(decays_to<Base> auto&& base)
        : map(FLUX_FWD(base), cast_to_const<const_element_t<Base>>{})
    {}

    struct flux_sequence_traits : map::flux_sequence_traits {
        static constexpr auto data(auto& self)
            requires contiguous_sequence<Base>
        {
            using P = std::add_pointer_t<std::remove_reference_t<const_element_t<Base>>>;
            return static_cast<P>(flux::data(self.base_)); // error: self.base_ is private
        }
    };
};

But that doesn't quite work. Another approach would be to have flux::read_only just return flux::map using the appopriate cast_to_const and just have map recognize it. That feels a little backwards (why is map adding a function that is only usable for read_only) but avoids all the boilerplate too.

[tcbrindle]

The test is easily fixable, so I'll do that (I think it dates from before I flipped the meaning of ref() and added mut_ref(), or maybe I just made a mistake).

Having read_only_adaptor inherit from map_adaptor is a neat idea (although I don't do it anywhere else). Does the access check work if you replace self.base_ with self.base()?

[brevzin]

The test is easily fixable, so I'll do that (I think it dates from before I flipped the meaning of ref() and added mut_ref(), or maybe I just made a mistake).

👍

Having read_only_adaptor inherit from map_adaptor is a neat idea (although I don't do it anywhere else). Does the access check work if you replace self.base_ with self.base()?

Oh yeah, that totally works! So yeah, my ... what... 25ish line implementation above there passes all the tests (with mut_ref fixed).

[tcbrindle]

I was literally just about to write that I've just tried it and it works! I'll have a PR in a couple of minutes