clayborg added a comment.
See inlined comments. Cool stuff.
================
Comment at: include/lldb/API/SBBreakpoint.h:26-27
+ SBBreakpoint(const lldb::BreakpointSP &bp_sp);
+
~SBBreakpoint();
----------------
Why does this need to be public?
================
Comment at: include/lldb/API/SBStructuredData.h:26-27
+
+ SBStructuredData(lldb_private::StructuredDataImpl *impl);
~SBStructuredData();
----------------
Why does this need to be public?
================
Comment at: include/lldb/API/SBSymbolContext.h:29-30
+ SBSymbolContext(const lldb_private::SymbolContext *sc_ptr);
+
~SBSymbolContext();
----------------
Why does this need to be public?
================
Comment at: include/lldb/lldb-enumerations.h:260-267
+ eSearchDepthInvalid = 0,
+ eSearchDepthTarget,
eSearchDepthModule,
eSearchDepthCompUnit,
eSearchDepthFunction,
eSearchDepthBlock,
eSearchDepthAddress,
----------------
Since this is public API we shouldn't change the enumeration values as this can
cause problems with LLDBRPC.framework. Change eSearchDepthInvalid to be -1 and
leave all other enums the same (including not renaming kNumSearchDepthKinds)?
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:4
+class Resolver:
+ got_files = 0
+ def __init__(self, bkpt, extra_args, dict):
----------------
Do you want this to be a class global? Seems like this belongs as an actual
member variable and should be initialized in the constructor
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:5-7
+ def __init__(self, bkpt, extra_args, dict):
+ self.bkpt = bkpt
+ self.extra_args = extra_args
----------------
Might be better to not do any work of consequence in the constructor and maybe
only give the breakpoint as the only arg? Maybe something like:
```
def __init__(self, bkpt):
self.bkpt = bkpt
self.args = None
def initialize(self, args):
self.args = args
# Check args and return an SBError if they are bad
return lldb.SBError()
```
This allows you to reject the arguments if they are invalid
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:9
+
+ def __callback__(self, sym_ctx):
+ sym_name = "not_a_real_function_name"
----------------
Do we want this to be named something besides the callback operator?
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:20-23
+ if sym_ctx.module.IsValid():
+ sym = sym_ctx.module.FindSymbol(sym_name, lldb.eSymbolTypeCode)
+ if sym.IsValid():
+ self.bkpt.AddLocation(sym.GetStartAddress())
----------------
Why do we need to manually add the breakpoint here? Can we just return True or
False from this function to ok the symbol context? Or is the idea that we might
move the address around a bit or add multiple locations give a single symbol
context?
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:28-31
+class ResolverModuleDepth(Resolver):
+ def __get_depth__ (self):
+ return lldb.eSearchDepthModule
+
----------------
Seems like "__get_depth__" should just be a normal method name. The double
underscore indicates this function is private. Any reason this needs to be
private?
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:33
+class ResolverCUDepth(Resolver):
+ def __get_depth__ (self):
+ return lldb.eSearchDepthCompUnit
----------------
ditto
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:37
+class ResolverBadDepth(Resolver):
+ def __get_depth__ (self):
+ return lldb.kLastSearchDepthKind + 1
----------------
ditto
================
Comment at: scripts/Python/python-wrapper.swig:358
+
+ lldb::SBBreakpoint *bkpt_value = new lldb::SBBreakpoint(breakpoint_sp);
+
----------------
Isn't the code generated by this internal to LLDB? Would be nice to not have to
make the shared pointer constructor for lldb::SBBreakpoint public if possible?
================
Comment at: scripts/Python/python-wrapper.swig:362
+
+ lldb::SBStructuredData *args_value = new lldb::SBStructuredData(args_impl);
+ PythonObject args_arg(PyRefType::Owned, SBTypeToSWIGWrapper(args_value));
----------------
Ditto about not making lldb::SBStructuredData(args_impl) public if possible?
================
Comment at: scripts/interface/SBBreakpoint.i:229-232
+ // Can only be called from a ScriptedBreakpointResolver...
+ SBError
+ AddLocation(SBAddress &address);
+
----------------
Do we enforce the limitation mentioned in the comment? If not, then remove the
comment? Else it would be great if this wasn't public. One solution to avoid
making the function public would be to have the Resolver.__callback__ function
return a list of lldb.SBAddress() objects. The internal LLDB code that made the
callback call could then add the locations to the breakpoint itself from inside
LLDB's private layer?
================
Comment at: scripts/interface/SBStringList.i:42
Clear ();
+
};
----------------
remove whitespace change
Repository:
rLLDB LLDB
https://reviews.llvm.org/D51830
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
