Mir

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

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

(1) Orphaned comment now needs removing:
16 # Why does this have to come after mir-test-framework here but not in tests/acceptance-tests/
17 # /CMakeLists.txt? ~racarr
18 - mirplatform

(2) Unnecessary (and undesirable) formatting changes:
221 -target_link_libraries(mirclient
222 +target_link_libraries(
223 + mirclient
224 +
because the first parameter is special and independent of those that follow it.

(3) Remind me why we wanted a C calling convention for C++ code?
372 +extern "C" std::shared_ptr<mcl::ClientPlatform>

(4) Minor optimization: You can avoid a template instantiation using std::atomic_bool for
898 + std::atomic<bool> connect_done;

(5) Fatal: Clients don't start any more:
$ bin/mir_demo_client_egltriangle
Can't get connection
$ bin/mir_demo_client_flicker
Starting
mir_demo_client_flicker: /home/dan/bzr/mir/tmp.pcd/examples/flicker.c:134: main: Assertion `mir_connection_is_valid(connection)' failed.

(6) ProbingClientPlatformFactory sounds a bit awkward. Although I haven't stopped to understand the code enough to suggest anything better yet.

(7) New (old) function exposed without a new stanza. Should be MIR_COMMON_3.1 (?)
980 +++ src/common/symbols.map 2014-11-28 05:44:33 +0000
981 @@ -150,6 +150,7 @@
982 # but they are used by libmirplatform, libmirclient or libmirserver
983 mir::default_server_socket;
984 mir::input::android::Lexicon::translate*;
985 + mir::libraries_for_path*;

review: Needs Fixing

« Back to merge proposal