- User Since
- Jun 4 2013, 6:02 AM (194 w, 4 d)
Fri, Feb 24
Switch SBBreakpoint to weak_ptr. This diff is pretty big but it amounts to
s/m_opaque_sp/m_opaque_wp and inserting a lock at the beginning of every
function. Since I had to touch them anyway, I took the opportunity to upgrade
the logging statements.
I am not sure if this is a voting situation, but I agree with what Zachary said above.
Looks great. Thank you.
Thu, Feb 23
No worries. I'll come back soon with the weak_ptr thingy.
That is true, however. It still seems that this property should be somehow tied to the child provider, because it is only in the presence of the provider that the unique_ptr obtains its magic dereferencing properties. For example, if I do a frame variable -R X Y, where X is a std::unique_ptr and Y is a variable that happens to have the same layout (but a different name) as X, I would expect them to be printed identically, but it seems this will break that.
I've tried this out on linux. I got it working only after making the following modifications:
Good stuff. We just need a couple of changes to make this conform better with llvm guidelines.
Jim's comment definitely makes sense. If we happen to not use pretty-printer for the unique_ptr, it will not behave as a pointer-like object, and it should be formatted as a normal struct (I have no idea how much of a challenge it would be to implement it that way).
Good stuff, just remove the unused function.
Wed, Feb 22
Note that the breakpoint location's site gets cleared when you disable the breakpoint or its location, as well as when you delete it. So if you have a workflow that does: "hit a breakpoint, disable it, hit another, reenable the first, hit the second" you'll end up re-evaluating the condition expression unnecessarily. This isn't a terribly common workflow, and compiling the expression is not hugely expensive. But in this instance, it was probably better to break the link at the SBBreakpoint level, and not here.
I think you are still waiting to get the llvm changes sorted out, but this side of it looks fine to me (modulo a couple of nits).
Adding Zachary as he was about to remove this code.
clang-format did not pick up the correct formatting because we have a .clang-format file in packages/Python/lldbsuite. This was necessary because a lot of the tests would get broken, as the formatting would break our // place breakpoint here annotations (also, stepping behavior is affected by the line breaks). It would be an interesting project to format all existing tests, but this would probably need to be done on a test-by-test basis (and may involve adding additional clang-format directives like CommentPragmas, ...).
Tue, Feb 21
I am sorry about the delay - I was busy last week and then this kinda fell off my radar.
Mon, Feb 20
Just found this lying on the bottom of my stack. Ed, if you don't object, I'd like to check in it. I think it is quite a safe change, so we probably don't need to do too much testing.
Seems to work for me after adding the magic.h include. I can't really guarantee that all bots will be happy with it though..
Fri, Feb 17
Thanks, I feel more comfortable with this one.
Thu, Feb 16
It does not seem too controversial, but I am not very confident reviewing lit changes. @beanz, could you take a look at this?
Really splitting hairs now, but could you also update the test to check for the return value. :)
Wed, Feb 15
The test added by this batch is failing on windows http://lab.llvm.org:8011/builders/lldb-windows7-android/builds/2499, for a very prosaic reason - we cannot run a "make clean" as something is holding the executable file open. Based on my debugging, it is not a problem in the test itself, but rather in the liblldb code (e.g. when I change all the breakpoint conditions to true, and adjust expectations accordingly, make clean suceeds). My feeling is something is creating a shared_ptr loop which prevents things from going away when the test is over.
Tue, Feb 14
Encapsulate the atomic stuff into the Log::Channel class. Add a couple of comments.
New version of the patch.
Using is_convertible will work right now. What will this prevent is someone
defining their own format_provider on a class which has a conversion operator
for StringRef. I've debated this with myself a bit. On one hand, it adds more
predictability (if something behaves enough like StringRef that in can be
converted to one, it should also be formatted like it). On the other hand, I'm
sure somebody somewhere will invent a case where it still makes sense to do
Mon, Feb 13
I actually did add tests for that. :)
Fri, Feb 10
Wed, Feb 8
Thank you for the review. I'll submit this tomorrow.
Ping. Any thoughts on this?
Tue, Feb 7
Don't be intimidated by the number of comments :) - I think the change looks reasonable as a whole - most of them are just about style and come from the fact you were probably working on the patch while I was refactoring this code. As for tests, what I'd definitely want to see is a lldb-server-style test, which tests just the server changes without the client part (maybe you we're already talking about those - I just want to make sure we're on the same page).