[Lldb-commits] [PATCH] D29359: Start breaking some dependencies in lldbUtility

Wed Feb 1 11:02:38 PST 2017

lgtm

On 1 February 2017 at 11:01, Zachary Turner <zturner at google.com> wrote:
> Correct, I'm not doing any of the "long term" stuff now, I was just thinking
> out loud with that.
>
> On Wed, Feb 1, 2017 at 10:49 AM Jim Ingham <jingham at apple.com> wrote:
>>
>> I don't think any of this should be terribly controversial.  The "Long
>> term" part of the proposals might be, but that's not what you're talking
>> about here, right?
>>
>> These changes will presumably require adjustments to the Xcode project.
>> If you can do that yourself, then that's great.  If you need one of us to
>> fix the projects, putting them up for review before checking in will give
>> one of us a chance to apply the patch and fix the projects and then we can
>> gang the two submissions so we don't leave trunk broken.  Other than that
>> I'm fine with you just checking in the changes.
>>
>> Jim
>>
>>
>> > On Jan 31, 2017, at 7:58 PM, Zachary Turner <zturner at google.com> wrote:
>> >
>> > Are you interested in seeing the followup patches for review (moving
>> > classes from Core -> Utility etc), or does it sound reasonable just based on
>> > my description?
>> >
>> > On Tue, Jan 31, 2017 at 6:05 PM Jim Ingham <jingham at apple.com> wrote:
>> > BTW, not to exclude the positive because it doesn't need discussing: all
>> > the rest of the changes you were proposing seem appropriate and good to me.
>> > Thanks for taking the trouble to clean this up.
>> >
>> > Jim
>> >
>> > > On Jan 31, 2017, at 5:45 PM, Zachary Turner <zturner at google.com>
>> > > wrote:
>> > >
>> > > Yea, there may be value in both. If i ever tried to do this I'd
>> > > probably take the approach of converting all the obvious cases first that
>> > > should enforce checking and then see what's left. Then we could use
>> > > llvm:Error and lldb::Status, or something
>> > > On Tue, Jan 31, 2017 at 5:39 PM Jim Ingham <jingham at apple.com> wrote:
>> > > Yeah, I have no idea how you'd make sure that the SBError returned to
>> > > Python in a Python SBValue was checked before it went out of scope, and I'm
>> > > not sure it's our business to be enforcing that...  So we're going to have
>> > > to do something special for those errors.  We also use the pattern where we
>> > > return a count and an error, but ofttimes you don't care why you got
>> > > nothing, so you just check the count.  The error is informational.  Making
>> > > that obnoxious would also not be a plus.  I actually think there's some
>> > > value to having "programming errors that must be checked" and informational
>> > > errors.  Maybe we need both.
>> > >
>> > > Jim
>> > >
>> > > > On Jan 31, 2017, at 5:32 PM, Zachary Turner <zturner at google.com>
>> > > > wrote:
>> > > >
>> > > > llvm::Error only enforces checking at the point that it goes out of
>> > > > scope. So the example you mention should be supported, as long as you
>> > > > propagate the value all the way up and don't destroy it. There's also ways
>> > > > to explicitly ignore an Error (similar to casting to void to make the
>> > > > compiler not warn about unused variables), so as a last resort we could put
>> > > > code like that in the SB implementation if we had to
>> > > > On Tue, Jan 31, 2017 at 5:23 PM Jim Ingham <jingham at apple.com>
>> > > > wrote:
>> > > > I think we discussed this before, but we need an informational error
>> > > > object.  IIRC, the llvm::Error has to be checked.  But for instance if you
>> > > > ask a value object to evaluate itself, but you've moved to a section of code
>> > > > where the variable has no location, then evaluating that value will result
>> > > > in an error.  But that isn't an error the value object code needs to do
>> > > > anything about.  And it might go all the way up through the SB & Python
>> > > > API's before it's appropriate to check the error.
>> > > >
>> > > > IIRC, the llvm::Error is one of those "you have to check it or you
>> > > > get smacked by the compiler" dealies.  That's appropriate for some of our
>> > > > uses of Error, but not all.
>> > > >
>> > > > Jim
>> > > >
>> > > >
>> > > > > On Jan 31, 2017, at 4:01 PM, Zachary Turner via Phabricator via
>> > > > > lldb-commits <lldb-commits at lists.llvm.org> wrote:
>> > > > >
>> > > > > zturner added a comment.
>> > > > >
>> > > > >> Move Error from Core -> Utility
>> > > > >
>> > > > > Also, I almost forgot.
>> > > > >
>> > > > > Long term: Delete and use `llvm::Error`
>> > > > >
>> > > > >
>> > > > > https://reviews.llvm.org/D29359
>> > > > >
>> > > > >
>> > > > >
>> > > > > _______________________________________________
>> > > > > lldb-commits mailing list
>> > > > > lldb-commits at lists.llvm.org
>> > > > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>> > > >
>> > >
>> >
>>
>