I'm not working on LLDB anymore. Sorry about that.

I don't work on LLDB anymore. I am sure Greg will do a fine job reviewing!

Thanks for doing this work.

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.

I don't work on LLDB anymore. Resigning as reviewer. Best of luck with your patch :-)

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

Chris, can you take a look at this? I am far from a CMake expert

Makes sense. Feel free to commit.

Make what you will of my suggestion above, but in general the reasoning looks good

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!

In D24591#543277, @beanz wrote:

@granata.enrico, we could migrate the existing tests into being executed by lit even if they aren't using lit's features, so if that direction is desired we could get everything in lit. That said, you shouldn't ever really have multiple incantations. Once we have reliable lit testing that is useful it should be connected to the "check-lldb" and "check-all" targets appropriately. Just because it runs more than one type of test doesn't mean you need multiple incantations, and more and varied testing is generally better for the quality of the product.

One piece of concern that I have is the multiplication of things that "testing LLDB" means.

LGTM

Looks good to me

Everything Greg said makes total sense

LGTM

LGTM

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

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?

This include is actually not necessary. I am removing it outright.

That seems reasonable, yes.

It looks like this code is default constructing a SBExpressionOptions(), which at the end of the day is a fancy wrapper for EvaluateExpressionOptions

Yes, I believe so.

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?

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.

This seems reasonable to me. I would wait for Greg to OK it, just to be safe.

I think it's OK to land this, yes

Looks good to me

Looks fine

Looks reasonable

That looks reasonable. Of course, I assume you ran the test suite on OS X, including your new evil test!

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?

Fine to get it in

Looks fine to me

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

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?

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

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

Sorry, I didn't realize you were still waiting on me to land this patch.

I reverted this in revision 248421 because it was causing breakages in the test suite.

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()

Fine by me

I am assuming you built this successfully.

Assuming it passes the test suite, LGTM

That looks reasonable

Can you please try using revision 243301 or later?