User Details
- User Since
- Oct 15 2014, 1:32 PM (396 w, 4 d)
Sep 30 2021
Feb 4 2020
Aug 23 2019
I'm not working on LLDB anymore. Sorry about that.
Oct 22 2018
Sep 2 2018
I don't work on LLDB anymore. I am sure Greg will do a fine job reviewing!
Jun 27 2018
Thanks for doing this work.
Jun 2 2017
On the surface, that looks correct to me. However I don't work on LLDB anymore and have been out of the loop enough that I don't feel comfortable approving this change. I believe Jim Ingham is Data Formatter Overlord these days.
Jan 24 2017
I don't work on LLDB anymore. Resigning as reviewer. Best of luck with your patch :-)
Nov 29 2016
I am not an Apple employee working on LLDB anymore
I am not an Apple employee working on LLDB anymore
I am not an Apple employee working on LLDB anymore
I am not an Apple employee working on LLDB anymore
Nov 16 2016
Chris, can you take a look at this? I am far from a CMake expert
Nov 9 2016
Makes sense. Feel free to commit.
Nov 8 2016
Make what you will of my suggestion above, but in general the reasoning looks good
Oct 19 2016
I see you already got a bunch of feedback on specific items. The overall idea looks good to me. I'll try to delve a little deeper in the code ASAP (I was out for a couple days and have some backlog...), but should be good to go assuming you address the feedback you already got + make sure tests work!
I see you already got a bunch of feedback on specific items. The overall idea looks good to me. I'll try to delve a little deeper in the code ASAP (I was out for a couple days and have some backlog...), but should be good to go assuming you address the feedback you already got + make sure tests work!
Sep 14 2016
One piece of concern that I have is the multiplication of things that "testing LLDB" means.
Aug 31 2016
Aug 29 2016
Jul 27 2016
Everything Greg said makes total sense
Jul 13 2016
Jul 5 2016
Jun 24 2016
LGTM
May 13 2016
Apr 29 2016
LGTM
Apr 28 2016
I don't think we are seeing this behavior on OS X (the failure to process sendline() calls) - but Todd Fiala might know more about that since he keeps a careful eye over our bots.
Sure, that works. My only concern is that I'd like to keep the current PExpect-let's test writing to the console and see what happens behavior
Apr 27 2016
My personal preference would be to split up the test cases. I'd really like to keep testing the immediate output to console scenario since it is interesting for - e.g. - kernel debugging at Apple.
Why are we trying to do this in the first place?
Apr 13 2016
Feb 25 2016
Feb 12 2016
This include is actually not necessary. I am removing it outright.
Jan 28 2016
That seems reasonable, yes.
Jan 27 2016
It looks like this code is default constructing a SBExpressionOptions(), which at the end of the day is a fancy wrapper for EvaluateExpressionOptions
Jan 21 2016
Yes, I believe so.
Jan 13 2016
Ops, sorry, didn't notice the tid_t type map was actually added back later on in the patch. My bad.
Did you mean to delete the tid_t typemap as well?
Dec 3 2015
IIU my C++ correctly, this code covers neither int64_t nor double; it covers the unsigned variety of int64 - as well as any other unsigned integer type.
Nov 9 2015
This seems reasonable to me. I would wait for Greg to OK it, just to be safe.
Oct 29 2015
I think it's OK to land this, yes
Oct 21 2015
Looks good to me
Oct 20 2015
Looks fine
Looks reasonable
That looks reasonable. Of course, I assume you ran the test suite on OS X, including your new evil test!
Oct 19 2015
Oct 16 2015
Oct 15 2015
One alternative would be to have the num_children method on the synthetic provider itself be able to take an optional argument that is the max_count. That is, instead of defining
So, if I understand this correctly, this HasChildAtIndex() API returns a three-state int:
If so, why do you need this to be exposed as a call that LLDB knows about?
Is it possible to ask for concrete examples?
What do you plan to use this has_child_at_index call for?
Looks fine to me
Oct 14 2015
So, if you do the explicit constructor change and handle the case of a nullptr Callback I think it should be good to go. Looking forward to it!
Is there any reason why we need a special command here?
Admittedly way simpler than my original idea. +1
Oct 13 2015
We are not supposed to be inheriting from SB classes, much less introduce virtual-ness to them (Greg can go in detail about the reasons, the tl;dr is that it has the potential to be ABI-breaking)
Currently I don't think SBTypeSummary allows for defining formatters backed by a CXXFunctionSummaryFormat
It would, however, be a great addition to our API. Is it a change you're interested in working on?
Oct 12 2015
Removing the size limit would be acceptable, yes
Possibility number 3 (and the true reason why the check is there): if you stop at a place where the variable is not fully initialized/being torn down, and as a result, something is pointing back inside the list. For a list traversal, that is a deadly outcome.
Why are you removing the loop detection?
The consistency argument is not entirely unfair.
Truth be told, I find this notation (numeric value + printable character) to be heavy and somewhat redundant
Oct 2 2015
I Thought I had marked it as good to go.
My bad if I have not.
I am not deeply involved in MI internals, so I am going to assume you ran the test suite, and that you actually verified that types without summaries still work
Oct 1 2015
Sorry, I didn't realize you were still waiting on me to land this patch.
Sep 23 2015
I reverted this in revision 248421 because it was causing breakages in the test suite.
Sep 22 2015
LGTM
My suggestion would be to add the DoesPrintValue() logic to SBTypeSummary. I can't recall if it's there already - but if not, it would be a fine thing to add. Then the MI could use SBValue and SBTypeSummary to make that determination as it sees fit.
Is there a reason to explicitly add an API to get the concatenation of value and summary?
Only thing I would suggest is using LLDB_INVALID_ADDRESS instead of 0 in GetArrayAddressOrPointerValue()
Sep 17 2015
Fine by me
I am assuming you built this successfully.
Aug 17 2015
Assuming it passes the test suite, LGTM
That looks reasonable
Jul 27 2015
Can you please try using revision 243301 or later?