Merge lp:~alan-griffiths/mir/improve-tidy-up-of-mir_connection_get_display_info into lp:mir

[Alan Griffiths]

client: RAII version of ~alan-griffiths/mir/lock_guard-for-unique_lock

client: RAII version of ~alan-griffiths/mir/lock_guard-for-unique_lock

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1121
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/143/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2305
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2190
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/140
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/140/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/143/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

I think this is a step backwards. The first hint is the increase in code size (+19/-9). But more importantly, this is difficult to read: unique_with_deleter. Much more difficult to read than the "mir_display_config_destroy(config)" you're aiming to replace. I would rather keep the code readable with mir_display_config_destroy.

RAII is not useful if it results in code that's less clear about the possibility of leaks than that which it replaces. And I think mir_connection_get_display_info is presently very clear on the lifetime of config.

Also, there are a few unrelated whitespace changes in src/client/mir_client_library.cpp.

review: Disapprove

[Alan Griffiths (alan-griffiths)]

> I think this is a step backwards. The first hint is the increase in code size
> (+19/-9). But more importantly, this is difficult to read:
> unique_with_deleter. Much more difficult to read than the
> "mir_display_config_destroy(config)" you're aiming to replace. I would rather
> keep the code readable with mir_display_config_destroy.

You're missing that it fixes the leaks caused by "return" statements that bypass that call. Alternative fixes also add lines.

> RAII is not useful if it results in code that's less clear about the
> possibility of leaks than that which it replaces. And I think
> mir_connection_get_display_info is presently very clear on the lifetime of
> config.

A RAII version was requested by Thomas and Alf - an alternative fix (that I was happy) with has already landed.

[Daniel van Vugt (vanvugt)]

There are no return statements. The flow of mir_connection_get_display_info is very clear as it's not a long function.

I favour explicit deletion over implicit, if the latter is significantly more cryptic (as it is here).

But other people may disagree.

[Alan Griffiths (alan-griffiths)]

> There are no return statements. The flow of mir_connection_get_display_info is
> very clear as it's not a long function.

There were in the version you referred to in "the increase in code size (+19/-9)." The alternative fix I mentioned removed them (-c 1120 on development-branch).

Explicit deletion is problematic in the presence of exceptions (which shouldn't apply here) or return statements (which we've just discussed).

It is reasonable to prefer uniform use of RAII for cleanup.

[Alexandros Frantzis (afrantzis)]

> RAII is not useful if it results in code that's less clear about the possibility of leaks
> than that which it replaces.

In this case (and in general) RAII minimizes the possibility of leaks now and in the future.

A reasonable alternative approach not using RAII, like r1120 on development-branch, is both more cumbersome than this and is not future-proof. What if the function gradually gets more complicated? What if someone inadvertently adds a return? We didn't catch this last time, we may not catch it the next.

Plus, we are using RAII/smart pointers all over the place, I don't see a reason to make an exception here.

review: Approve

[Daniel van Vugt (vanvugt)]

I don't think one can talk about "future proof" at the same time as making code less readable. If it's less readable, you're cursing future maintainers to either not comprehend the code (so can't maintain it and/or waste time), or to misunderstand it and introduce regressions. Leaks are not the only kind of bug, and having readable code is a better preventative measure. In fact, leaks are much less important and less severe than many of the conceivable outcomes of a future maintainer not understanding the code.

[Kevin DuBois (kdub)]

I think this is okay. Perhaps it would be more immediately readable if lines 51-53 were just a shared_ptr with a deleter though (less figuring out what the inlined function is doing). I do think raii is the right way to fix the problem, but can empathize with wanting to figure out less types of pointers.

[kevin gunn (kgunn72)]

granted i don't touch code everyday, but it does seem to me that RAII is an accepted paradigm that was created to avoid orphaning objects from an owning objects excpetion (...a good thing)
at the same time, Could some additional pointed comments be added to the code to solve the maintainability concern ?

note...i don't agree that we should look at code size generically as a measuring stick...i've certainly seen it go the other way where people trying to "shrink" code made it more enigmatic.

I would propose some comments to be added as to what the deleter is doing, how it helps in the exception instance...then call this good

[Daniel van Vugt (vanvugt)]

My disapproval is only about style and readability. Please treat it like "Abstain" so we don't waste any more time arguing. I fully expect this to land still.

[Alan Griffiths (alan-griffiths)]

I'm hearing that some folks would prefer losing the helper function and:

    std::unique_ptr<MirDisplayConfiguration, decltype(&mir_display_config_destroy)> const config(

to:

    auto const config = unique_with_deleter(

I guess it is a trade off between syntactic noise and an unfamiliar helper function. Let me know.

=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2013-10-17 05:51:24 +0000
+++ src/client/mir_client_library.cpp	2013-10-17 10:51:13 +0000
@@ -289,54 +289,60 @@
     mcl::delete_config_storage(configuration);
 }
 
+namespace
+{
+template <typename Owned>
+inline std::unique_ptr<Owned, void(*)(Owned*)> unique_with_deleter(Owned* owned, void(*deleter)(Owned*))
+{
+    return {owned, deleter};
+}
+}
+
 //TODO: DEPRECATED: remove this function
 void mir_connection_get_display_info(MirConnection *connection, MirDisplayInfo *display_info)
 {
-    auto config = mir_connection_create_display_config(connection);
-
-    do
-    {
-        if (config->num_outputs < 1)
-            break;
-
-        MirDisplayOutput* state = nullptr;
-        // We can't handle more than one display, so just populate based on the first
-        // active display we find.
-        for (unsigned int i = 0; i < config->num_outputs; ++i)
-        {
-            if (config->outputs[i].used && config->outputs[i].connected &&
-                config->outputs[i].current_mode < config->outputs[i].num_modes)
-            {
-                state = &config->outputs[i];
-                break;
-            }
-        }
-        // Oh, oh! No connected outputs?!
-        if (state == nullptr)
-        {
-            memset(display_info, 0, sizeof(*display_info));
-            break;
-        }
-
-        MirDisplayMode mode = state->modes[state->current_mode];
-
-        display_info->width = mode.horizontal_resolution;
-        display_info->height = mode.vertical_resolution;
-
-        unsigned int format_items;
-        if (state->num_output_formats > mir_supported_pixel_format_max)
-             format_items = mir_supported_pixel_format_max;
-        else
-             format_items = state->num_output_formats;
-
-        display_info->supported_pixel_format_items = format_items;
-        for(auto i=0u; i < format_items; i++)
-        {
-            display_info->supported_pixel_format[i] = state->output_formats[i];
-        }
-    } while (false);
-
-    mir_display_config_destroy(config);
+    auto const config = unique_with_deleter(
+        mir_connection_create_display_config(connection),
+        &mir_display_config_destroy);
+
+    if (config->num_outputs < 1)
+        return;
+
+    MirDisplayOutput* state = nullptr;
+    // We can't handle more than one display, so just populate based on the first
+    // active display we find.
+    for (unsigned int i = 0; i < config->num_outputs; ++i)
+    {
+        if (config->outputs[i].used && config->outputs[i].connected &&
+            config->outputs[i].current_mode < config->outputs[i].num_modes)
+        {
+            state = &config->outputs[i];
+            break;
+        }
+    }
+    // Oh, oh! No connected outputs?!
+    if (state == nullptr)
+    {
+        memset(display_info, 0, sizeof(*display_info));
+        return;
+    }
+
+    MirDisplayMode mode = state->modes[state->current_mode];
+
+    display_info->width = mode.horizontal_resolution;
+    display_info->height = mode.vertical_resolution;
+
+    unsigned int format_items;
+    if (state->num_output_formats > mir_supported_pixel_format_max)
+         format_items = mir_supported_pixel_format_max;
+    else
+         format_items = state->num_output_formats;
+
+    display_info->supported_pixel_format_items = format_items;
+    for(auto i=0u; i < format_items; i++)
+    {
+        display_info->supported_pixel_format[i] = state->output_formats[i];
+    }
 }
 
 void mir_surface_get_graphics_region(MirSurface * surface, MirGraphicsRegion * graphics_region)