LGTM too. Thanks for taking the time to address the comments.

LG. If you can add a line to an existing inline test, I think it would be excellent.

I'll commit this for you, but I might ask if you can try adding a test first?

Some basic comments. Haven't looked at the implementation very closely, I'll do that probably tomorrow. Thanks for working on this!

I think this looks fine. Vedant, thoughts?

LGTM.

This broke all the lldb bots as well, FWIW.

Sure.

LGTM modulo minor.

Also, thanks for taking the time to do this right :)

LGTM. @zturner / @labath might have opinions.

How exactly are you building (i.e. what triggers this error)?

Yeah, sure :)

Yeah, that's the revision I had in mind. Can't you extend lldb-test to do what you need?

I thought Vedant added an harness utility to test memory a bit ago. Have you looked into whether that's extensible for your purposes, Raphael?

Can you add a test for this behavior?

Cool. The argument about reverting still stands though (you might
consider following up with logs et simila to make debugging easier).

You can probably revert it. BTW, is the bot public? I would like to
understand whether I broke something after I commit.

LGTM. @eraman / @davidxl might have opinions on this.

I'm not familiar enough with MLSM to review this, so @Gerolf should take a look.

lgtm

I agree.

LGTM

LGTM.

LGTM if Vedant is happy with this.

I think this is fine, but I would be happier if Eli can take a look, as I'm not particularly familiar with this.

Do you have numbers of the size win? (maybe on clang/llvm itself)

LGTM.

I never managed to review this one, but I saw it was committed anyway. I agree the style is inconsistent, though the patch logic is fine.

Yeah, this is much more in line with what I had in mind than the original patch. Thanks for taking care of this!

LGTM as discussed in person.

Looks reasonable to me.

Lgtm.

Sure.

My understanding is that @t.p.northover just committed the same patch.

LGTM, thanks George. I'm not particularly worried if it's too hard to write a test for this.

LGTM.

Bot logs for completeness:

I'm afraid I had to revert this again. It broke an ubuntu lldb-cmake bot which builds with clang-3.5 (which has no -std=c++17 support flag, as 17 wasn't there yet).

a11963323fc Recommit [DataFormatters] Add formatter for C++17 std::optional.
Authentication realm: https://llvm.org:443 LLVM Subversion repository
Password for 'davide': ***

Your latest update doesn't contain CMakeList.txt files and the Xcode project changes. That's why it failed to apply. Please upload a correct version and I'll commit it for you.

Although I'm not entirely sure whether this is safe (as in, it doesn't break anything), it's probably not worth to block this further. My understanding is that Raphael did a bunch of testing and he's willing to follow-up on eventual regressions. So, given this passes the test suite, I think it's OK for this patch to go in.

LG. thanks for improving the interface, I think all these cleanups are really good.

lg. You probably don't need pre-commit reviews for adding tests. This is obvious goodness.

This is good, but please add a comment explaining the type before committing.

lg

In D49617#1170597, @hiraditya wrote:

In D49617#1170536, @davide wrote:

This is probably fine, but does it matter? i.e. you have compile time numbers?

I dont have numbers, maybe you can help with that.

Thanks for doing this.
We may consider doing some A-B testing between the two demanglers.
A strategy that worked very well for similar purposes was that of running nm on a large executable (e.g. clang or lldb itself) and see whether we demangle in the same exact way and measure the time needed for demangling.

This is probably fine, but does it matter? i.e. you have compile time numbers?

lgtm.

Can you add a unittest for the new functionality?

I guess this is fine. @jingham?

LGTM modulo minor. Thanks.

Thanks Teresa.

Probably last round of comments. Thanks for your patience!

LGTM modulo minor.

FWIW, my take is that we should kill ResolvedUndefsIn (as already pointed by Eli, and probably me, and others in the past).
My take is that the benefit of having a forth lattice state is really little compared to the amount of effort we had to put to fix bugs.
If somebody is willing to take the time to do the work I'd rather zap away undef from the lattice and see how many constant we lose (and what's the runtime cost). But I understand this is a fair amount of work. That said, I think I like Eli's proof of concept a little better than this, so I'm happy if you want to pursue that path, Florian.

This is getting really close. Please try the lldbInline test format and revert the unrelated bits and I'll take another look.
Thanks!