Code review comment for lp:~raof/mir/probe-client-drivers

On Wed, Dec 10, 2014 at 2:44 PM, Daniel van Vugt
<email address hidden> wrote:
> Review: Needs Fixing
>
> (2) Unnecessary (and undesirable) formatting changes:
> 575 -target_link_libraries(mirclient
> 576 +target_link_libraries(
> 577 + mirclient
> 578 +
> because the first parameter is special and independent of those that
> follow it.
>
> (4) Minor optimization: You can avoid a template instantiation using
> std::atomic_bool for
> 1250 + std::atomic<bool> connect_done;
>
> (8) The library re-ordering appears to be pointless, or is there a
> reason?...
> benchmarks/frame-uniformity/CMakeLists.txt
> Can we revert all changes to that file?
>
> (9) Clients now start, but the wrapper that makes it possible feels
> inelegant...
> $ ls -a bin/
> mir_acceptance_tests
> .mir_acceptance_tests-uninstalled
> mir_demo_client_basic
> .mir_demo_client_basic-uninstalled
> mir_demo_client_cursors
> .mir_demo_client_cursors-uninstalled
> mir_demo_client_display_config
> .mir_demo_client_display_config-uninstalled
> mir_demo_client_eglcounter
> .mir_demo_client_eglcounter-uninstalled
> ...
> I suspect we can do better without needing a wrapper. But if we do
> use this then please don't hide binaries from developers:
> ".foo-uninstalled" should be "foo.bin"

I think we need one of (a) a wrapper, or (b) code in the libraries we
release solely there to negate the need for a trivial wrapper.

Choosing (b) introduces the possibility of otherwise-unnecessary code
having user-visible bugs, so I chose (a).

I hid the real binaries in the time-honoured tradition of libtool
(which uses wrapper scripts, and hides everything in a .libs/
directory). You've got a choice between developers going “why are
there all these *.bin that don't work when I try to run them lying
around in my build tree” and “why are there all these hidden
binaries lying around in my build tree”.

I think hidden files are less confusing. The (normal) directory listing
lists everything you should be running.

> (10) wrapper.c compiles to 10KB, but could be replaced with a trivial
> shell script instead.

Said shell script makes it more difficult to run gdb which is why I
switched to wrapper.c.

>
> (11) This is new on the end. What's it do?
> 1343 +} MIR_COMMON_3;

“MIR_COMMON_3.1 depends on MIR_COMMON_3”