> On Oct 14, 2014, at 5:50 AM, Carlo Kok <c...@remobjects.com> wrote: > > > > jing...@apple.com schreef op 10/13/2014 om 8:17 PM: >> This looks good. A couple of trivial things, >> >> 1) You make a shared pointer to a ClangFunction: >> >> std::shared_ptr<ClangFunction>& func... >> >> We usually append "_sp" to shared pointers so you can tell that their >> lifespan is different from ordinary stack objects. > > ke. > >> >> 2) Also, when there are errors evaluating the function you return an >> empty SBValue. But SBValues can have errors (returned with GetError, >> though only settable on the ValueObject side... Might be nice to set >> the error for the SBValue rather than just returning an empty one. > > sounds good. > >> 3) You have RemoveReusableFunction but it doesn't look like you >> protected against somebody removing a reusable function while >> somebody else was using it. > > a simple target lock around both should do there? > >> >> 4) SBTypeMemberFunction::ExecuteFunction is explicitly ObjC only. >> But the name doesn't express that (except that you call the parameter >> for the implicit object "self") and you don't enforce that self is an >> ObjC object anywhere. Be good to make that clear. > > Does this actually get returned for anything else? the SBTypeMemberFunction? > If it is I'm not sure how to call this for other languages. I'll rename them > to ExecuteObjcFunction and put a check in it for the next version of this > patch.
Even if only ObjC returns this now, there's nothing anywhere that says that is true, so it's better to be explicit... And it would be great to add a test case too! Thanks for working on this, I think it will be useful. Jim > > -- > Carlo Kok > RemObjects Software _______________________________________________ lldb-dev mailing list lldb-dev@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
