Re: [PATCH] 646698 Add cmd line option to choose the gdb binary

[Dodji Seketeli <dodji seketeli org>]

Hello Fabien,

I just have a few nit comments about this patch that is otherwise OK.

diff --git a/src/dbgengine/nmv-gdb-engine.cc b/src/dbgengine/nmv-gdb-engine.cc
index 29534e6..42d6dbd 100644
--- a/src/dbgengine/nmv-gdb-engine.cc
+++ b/src/dbgengine/nmv-gdb-engine.cc
@@ -224,6 +224,7 @@ public:
     int cur_thread_num;
     Address cur_frame_address;
     ILangTraitSafePtr lang_trait;
+    UString cmdline_debugger_path;

Please name this non_persistent_debugger_path.

     UString debugger_full_path;
     UString follow_fork_mode;
     UString disassembly_flavor;
@@ -422,10 +423,14 @@ public:
 
     const UString& get_debugger_full_path () const
     {
+        const_cast<Priv*> (this)->debugger_full_path = cmdline_debugger_path;
+

I think we should add a "mutable" specifier to the declaration of
debugger_full_path so that we can get rid of the const_cast<Priv*>
here.  Please get rid of the const_cast<Priv*> that was initially in
the function as well.

         NEMIVER_TRY
-        get_conf_mgr ()->get_key_value (CONF_KEY_GDB_BINARY,
-                                        const_cast<Priv*>
+        if (debugger_full_path.empty()) {
+            get_conf_mgr ()->get_key_value (CONF_KEY_GDB_BINARY,
+                                            const_cast<Priv*>
                                                 (this)->debugger_full_path);

The const_cast should be useless if debugger_full_path becomes mutable.

+        }
         NEMIVER_CATCH_NOX
 
         if (debugger_full_path == "" ||
@@ -4044,6 +4049,13 @@ GDBEngine::busy () const
     return false;
 }
 
+void
+GDBEngine::set_debugger_full_path (const UString &a_full_path)

Please update this function name to set_non_persistent_debugger_path.

+{
+    THROW_IF_FAIL (m_priv);
+    m_priv->cmdline_debugger_path = a_full_path;
+}
+
 const UString&
 GDBEngine::get_debugger_full_path () const
 {
diff --git a/src/dbgengine/nmv-i-debugger.h b/src/dbgengine/nmv-i-debugger.h
index 4e8b2ed..a8a9c2c 100644
--- a/src/dbgengine/nmv-i-debugger.h
+++ b/src/dbgengine/nmv-i-debugger.h
@@ -1113,6 +1113,8 @@ public:
 
     virtual bool busy () const = 0;
 
+    virtual void set_debugger_full_path (const UString &a_full_path) = 0;
+

Let's name this set_non_persistent_debugger_path, rather.

This patch is OK to commit to master and gtk2-branch with those changes.

Thank you for your time!

-- 
		Dodji