Code review comment for lp://staging/~nick-dedekind/qtmir/miral-DisplayConfigurationStorage

> > + if (output_index == config.clone_output_index.value()) {
> > + output.top_left = find_output.top_left;
> > break again?
> Can't break out of a for_each_output callback

You're right, you're in a lambda. You could "return" instead, but that does look confusing in code. Best to leave it.

> > + if (config.orientation.is_set()) {output.orientation =
> > config.orientation.value(); }
> > + if (config.used.is_set()) {output.used =
> config.used.value();
> > }
> > + if (config.form_factor.is_set()) {output.form_factor =
> > config.form_factor.value(); }
> > + if (config.scale.is_set()) {output.scale = config.scale.value(); }
> >
> > What if these values were set, and then we want to unset them? Does that
> make
> > sense?
> >
> > This is all wrapped in a giant try/catch block, why?
>
> edid.parse_data throws an exception if it encounters bad data.
> Would log out something, but this is qtmir::miral so wasn't sure what we do.

Ok, my first instinct is to only to wrap parse_data call. But if the exception throws, then you naturally skip all the config saving bits, which is clean. So ok.

> > + // FIXME - output.edid should be std::vector<uint8_t>, not
> > std::vector<uint8_t const>
> > + edid.parse_data(reinterpret_cast<std::vector<uint8_t>
> const&>(output.edid));
> > IMO std::vector<uint8_t const> is more correct, why would we want the vector
> > contents to be editable? miral::Edid should take the const one instead.
>
> Apparently vector elements can't be const :/ not sure why it's constructable,
> but it's not usable. Get a compiler error. something to do with the allocator.

There is something fishy here. http://en.cppreference.com/w/cpp/container/vector says that the type in a vector (for C++11 and greater) needs to just be Erasable, which const uint8_t should be. But yeah, compiler says no. Better leave as is.