abhishek.aggarwal added a comment.
Hi Greg
Please find my comments inlined. Let me know if I am missing something here.
================
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);
----------------
clayborg wrote:
> You can't change public API, you can only add to it. Just add another
> function with syntax and leave the other one alone.
After reading your review, I suggest to keep the following prototype of this
function:
AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char*
help, const char* syntax = nullptr);
This will not break anyone's plugins written with old version of lldb shared
library as syntax will be an optional argument.
This way, we will not need to add another API in this class thereby keeping the
public APIs as minimal as possible and complete at the same time.
Please let me know if this fits our public API development conditions. Else I
will add another variant of AddCommand API with syntax argument.
================
Comment at: include/lldb/API/SBCommandInterpreter.h:270
@@ -269,2 +269,3 @@
char** /*command*/,
+ size_t /*number of arguments*/,
lldb::SBCommandReturnObject & /*result*/)
----------------
clayborg wrote:
> 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.
I will do it.
================
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);
----------------
clayborg wrote:
> You can't change public API, you can only add to it. Just add another
> function with syntax and leave the other one alone.
Same comment as for SBCommandInterpreter::AddCommand API above
This will not break anyone's plugins written with old version of lldb shared
library as syntax is an optional argument.
This way, we will not need to add another API in this class thereby keeping the
public APIs as minimal as possible and complete at the same time.
================
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)
----------------
clayborg wrote:
> revert
Will do it.
================
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();
----------------
clayborg wrote:
> Remove count, revert this line.
Will do it.
================
Comment at: source/API/SBCommandInterpreter.cpp:156
@@ -155,3 +155,3 @@
}
- lldb::SBCommandPluginInterface* m_backend;
+ std::shared_ptr<lldb::SBCommandPluginInterface> m_backend;
};
----------------
clayborg wrote:
> 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.
Few points which I want to say here:
1. class CommandPluginInterfaceImplementation is completely a part of
implementation file and is not present in any public interface file. Hence, any
API of this class will never be exposed to lldb shared library users.
2. I referring to
packages/Python/lldbsuite/test/functionalities/plugins/commands/plugin.cpp file
as an example.
In lldb::PluginInitialize() function the ChildCommand instance is
dynamically allocated and a pointer to this instance will be copied to
m_backend data member of CommandPluginInterfaceImplementation class. However,
no one is freeing this dynamically allocated memory leading to memory leaks. I
believe anyone who would have written plugins using this approach will be
leaking memory as I just explained. In my opinion it is a bug and for this we
need to change this pointer to a shared pointer.
Please let me know if I am missing something here.
================
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)
{
----------------
clayborg wrote:
> You can't change public API, you can only add to it. Just add another
> function with syntax and leave the other one alone.
Same Comment as in interface file of this class.
AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char*
help, const char* syntax = nullptr); might solve this issue.
================
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)
{
----------------
clayborg wrote:
> You can't change public API, you can only add to it. Just add another
> function with syntax and leave the other one alone.
Same Comment as in interface file of this class for this function.
https://reviews.llvm.org/D22863
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
