[Lldb-commits] [PATCH] D22863: Improve code of loading plugins that provide cmnds

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

You can't change the exiting public API (anything in include/lldb/API/*). If you want to a add a new function, you must add a new version with the new argument, but leave the old one alone. See the following link for details:

http://lldb.llvm.org/architecture/index.html#api

SBCommandPluginInterface is special because it does have virtual functions. This means you can't change it or any of the virtual functions. So all changes to this class must be reverted since old code might have linked against the old version and still be compiled for the old version.

A simpler fix for all of this could be to just add a:

lldb::SBCommand::SetSyntax(const char *);

Then you wouldn't need to modify any other classes. But you can add new variants that include syntax to the AddCommand() function if you want.


================
Comment at: include/lldb/API/SBCommandInterpreter.h:141-142
@@ -140,4 +140,4 @@
     
     lldb::SBCommand
-    AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help);
+    AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help, const char* syntax);
 
----------------
You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.

================
Comment at: include/lldb/API/SBCommandInterpreter.h:270
@@ -269,2 +269,3 @@
                char** /*command*/,
+               size_t /*number of arguments*/,
                lldb::SBCommandReturnObject & /*result*/)
----------------
Since this is a special class that does use virtual functions that is in the public API, you can't change it. Previous code might have linked against the LLDB shared library and changing this would break those plug-ins and they would install their new implementation in the vtable slot for this function and it would be called incorrectly. Revert this change. 

================
Comment at: include/lldb/API/SBCommandInterpreter.h:310
@@ -308,3 +309,3 @@
     lldb::SBCommand
-    AddCommand(const char* name, lldb::SBCommandPluginInterface* impl, const char* help = nullptr);
+    AddCommand(const char* name, lldb::SBCommandPluginInterface* impl, const char* help = nullptr, const char* syntax = nullptr);
     
----------------
You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.

================
Comment at: packages/Python/lldbsuite/test/functionalities/plugins/commands/plugin.cpp:37
@@ -36,2 +36,3 @@
                char** command,
+               size_t arg_count,
                lldb::SBCommandReturnObject &result)
----------------
revert

================
Comment at: source/API/SBCommandInterpreter.cpp:152
@@ -151,3 +151,3 @@
         SBDebugger debugger_sb(m_interpreter.GetDebugger().shared_from_this());
-        bool ret = m_backend->DoExecute (debugger_sb,(char**)command.GetArgumentVector(), sb_return);
+        bool ret = m_backend->DoExecute (debugger_sb,(char**)command.GetArgumentVector(),command.GetArgumentCount(), sb_return);
         sb_return.Release();
----------------
Remove count, revert this line.

================
Comment at: source/API/SBCommandInterpreter.cpp:156
@@ -155,3 +155,3 @@
     }
-    lldb::SBCommandPluginInterface* m_backend;
+    std::shared_ptr<lldb::SBCommandPluginInterface> m_backend;
 };
----------------
Can't change public API, revert this. This would break anyone's existing commands as the layout of this class would change and possibly break and code that links against this.

================
Comment at: source/API/SBCommandInterpreter.cpp:598
@@ -597,3 +597,3 @@
 lldb::SBCommand
-SBCommandInterpreter::AddCommand (const char* name, lldb::SBCommandPluginInterface* impl, const char* help)
+SBCommandInterpreter::AddCommand (const char* name, lldb::SBCommandPluginInterface* impl, const char* help, const char* syntax)
 {
----------------
You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.

================
Comment at: source/API/SBCommandInterpreter.cpp:667
@@ -666,3 +666,3 @@
 lldb::SBCommand
-SBCommand::AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help)
+SBCommand::AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help, const char* syntax)
 {
----------------
You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.


https://reviews.llvm.org/D22863