- User Since
- Nov 12 2014, 1:58 PM (205 w, 4 d)
Fri, Oct 12
Thu, Oct 11
LGTM too. Thanks for taking the time to address the comments.
Mon, Oct 8
LG. If you can add a line to an existing inline test, I think it would be excellent.
Wed, Oct 3
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?
Fri, Sep 28
Tue, Sep 25
Sep 19 2018
This broke all the lldb bots as well, FWIW.
Sep 18 2018
LGTM modulo minor.
Sep 17 2018
Sep 14 2018
Also, thanks for taking the time to do this right :)
Sep 12 2018
How exactly are you building (i.e. what triggers this error)?
Sep 11 2018
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?
Sep 10 2018
Sep 7 2018
Can you add a test for this behavior?
Sep 6 2018
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.
Sep 5 2018
I'm not familiar enough with MLSM to review this, so @Gerolf should take a look.
Sep 4 2018
Aug 31 2018
Aug 28 2018
Aug 27 2018
LGTM if Vedant is happy with this.
Aug 25 2018
Aug 24 2018
Aug 23 2018
I think this is fine, but I would be happier if Eli can take a look, as I'm not particularly familiar with this.
Aug 20 2018
Do you have numbers of the size win? (maybe on clang/llvm itself)
Aug 19 2018
Aug 9 2018
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!
Aug 8 2018
LGTM as discussed in person.
Looks reasonable to me.
Aug 5 2018
Aug 2 2018
My understanding is that @t.p.northover just committed the same patch.
Aug 1 2018
LGTM, thanks George. I'm not particularly worried if it's too hard to write a test for this.
Jul 30 2018
Jul 27 2018
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.
Jul 26 2018
LG. thanks for improving the interface, I think all these cleanups are really good.
Jul 25 2018
Jul 23 2018
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.
Jul 20 2018
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?
Jul 19 2018
Can you add a unittest for the new functionality?
I guess this is fine. @jingham?
Jul 18 2018
LGTM modulo minor. Thanks.
Jul 17 2018
Probably last round of comments. Thanks for your patience!
Jul 16 2018
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.
Jul 13 2018
This is getting really close. Please try the lldbInline test format and revert the unrelated bits and I'll take another look.