Merge lp://qastaging/~vanvugt/mir/output-type-names into lp://qastaging/mir

PASSED: Continuous integration, rev:3696
https://mir-jenkins.ubuntu.com/job/mir-ci/1679/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2104
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2166
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2157
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2157
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2157
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2132
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2132/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2132
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2132/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2132
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2132/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2132
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2132/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2132
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2132/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1679/rebuild

PASSED: Continuous integration, rev:3698
https://mir-jenkins.ubuntu.com/job/mir-ci/1686/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2111
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2173
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2164
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2164
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2164
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2139
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2139/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2139
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2139/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2139
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2139/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2139
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2139/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2139
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2139/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1686/rebuild

nits, lgtm otherwise:

+ * \param [in] t The MirDisplayOutputType to describe.

I'd rather just have 'type' because then I wouldn't have to figure out what 't' means.

+ return nullptr;
why not return 'unknown enum'

+ * \param [in] t The MirOutputType to describe.
+ * \returns The name of the output type.
+ */
+char const* mir_output_type_name(MirOutputType t);

Abbreviating the parameter to "t" isn't descriptive. As Kevin says "type" would be clearer.

(1) I'm very surprised two people are suggesting that t is unclear in "MirOutputType t", but don't mind changing.

(2) Indeed 'return nullptr' used to be 'return "out of range"'. However I changed it to nullptr for two reasons: (a) If a user then saw a message "out of range" it would be out of context and confusing as to what is out of range; and (b) It's a fatal error if we forget to update the table, so crashing is preferred. I think.

PASSED: Continuous integration, rev:3700
https://mir-jenkins.ubuntu.com/job/mir-ci/1703/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2131
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2193
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2184
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2184
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2184
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2159
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2159/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2159
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2159/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2159
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2159/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2159
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2159/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2159
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2159/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1703/rebuild

I tend to agree with Kevin that "return nullptr" is failing to meet the postcondition and with Daniel that out of range is failing to meet a precondition (so we don't need to).

How about calling mir::fatal_error() instead?

In certain usage scenarios return nullptr would be ignored - i.e. printf might turn that into a "null" string which would lead to a similar argument as the one against "out of range" .. so I would also prefer fatal_error()

I'm going to put in a dissenting opinion, and say that we should return “unknown” when we can't find a matching output type name.

fatal_error() would be correct if this were only used in the server where we can guarantee we only set known values, but it's incorrect in the client where the values come from the server.

Needs-fixing for that.

Non-blocking nits:
*) Why not char const* mir_output_get_type_name(MirOutput const* output)?
*) If not that, then please remove mir_output_type_name(MirDisplayOutputType type) - there's no non-deprecated¹ way of getting a MirDisplayOutputType

¹: OK, not soon-to-be-deprecated.

(1) Returning "unknown"/"out of range" is wrong for reasons mentioned yesterday. Arguably we should document that the client functions potentially return NULL, but actually they don't. If the client API functions ever returned NULL that would be a programming error on our part (or some devious casting by the client). Because the parameters to the client function are strongly typed -- you can't get a NULL result unless you violate that type (or we add a new type in the server and forget to implement it, which is a fatal error we want to know about through crash reports and fix immediately). So returning NULL is correct, but also not something that should be documented for users to expect.

(2) mir_output_get_type_name(MirOutput const* output) would also be wrong because it involves passing in more information than the function needs, without gaining any additional type safety. You only lose flexibility (if say the client is storing MirOutputType's and just wants to display their names later).

(3) I agree adding mir_display_output_type_name(MirDisplayOutputType type) is not perfect as it will soon get deprecated with that old API. However it is necessary right now to complete the de-duplication work (see src/utils/out.c). Although we could do a devious typecast there and use the new API, I think introducing a properly typed function and then soon deprecating it is cleaner.

On Wed, Sep 14, 2016 at 2:15 PM, Daniel van Vugt
<email address hidden> wrote:
> (1) Returning "unknown"/"out of range" is wrong for reasons mentioned
> yesterday. Arguably we should document that the client functions
> potentially return NULL, but actually they don't. If the client API
> functions ever returned NULL that would be a programming error on our
> part (or some devious casting by the client). Because the parameters
> to the client function are strongly typed -- you can't get a NULL
> result unless you violate that type (or we add a new type in the
> server and forget to implement it, which is a fatal error we want to
> know about through crash reports and fix immediately). So returning
> NULL is correct, but also not something that should be documented for
> users to expect.

You're absolutely correct - mismatched values should be handled at the
IPC layer. Which is why we should abort() here, rather than hoping the
client crashes immediately.
>
> (3) I agree adding mir_display_output_type_name(MirDisplayOutputType
> type) is not perfect as it will soon get deprecated with that old
> API. However it is necessary right now to complete the de-duplication
> work (see src/utils/out.c). Although we could do a devious typecast
> there and use the new API, I think introducing a properly typed
> function and then soon deprecating it is cleaner.

I disagree, but the function is sufficiently trivial that I don't
disagree enough to immediately propose a branch removing it :).