Code review comment for lp://qastaging/~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.

"Erasable, but many member functions impose stricter requirements."
Perhaps this is why constructing one is fine in mir but actually using it in our case is where the "Compiler Says No"