This is an archive of the discontinued LLVM Phabricator instance.

Convert some Breakpoint to StringRef
ClosedPublic

Authored by zturner on Oct 1 2016, 8:14 AM.

Details

Summary

This is an incremental step towards getting some other code converted. In any case, I believe this makes the breakpoint code significantly easier to understand and also removes many string copies in the range id code.

Diff Detail

Event Timeline

zturner updated this revision to Diff 73197.Oct 1 2016, 8:14 AM
zturner retitled this revision from to Convert some Breakpoint to StringRef.
zturner updated this object.
zturner added a reviewer: jingham.
zturner added a subscriber: lldb-commits.
jingham accepted this revision.Oct 3 2016, 7:12 PM
jingham edited edge metadata.

This looks correct to me. I had a couple of trivial inline comments, but this looks fine.

source/Breakpoint/BreakpointID.cpp
80–81

This really should return a BreakpointID not pointers to the break & loc ID's. It always gets used that way, not sure why it wasn't written that way... That's not a necessary change but while you are cleaning this stuff up...

source/Breakpoint/BreakpointIDList.cpp
350–351

Can you add a comment here saying what is returned, particularly that you return a pair of empty string refs when it isn't an ID range?

This revision is now accepted and ready to land.Oct 3 2016, 7:12 PM
zturner updated this revision to Diff 73554.Oct 4 2016, 2:21 PM
zturner edited edge metadata.

Fixed up ParseCanonicalReference and SplitIDRangeExpression as suggested. I could have probably returned an Optional<std::pair<StringRef,StringRef>> but it seemed like overkill. An empty pair seems sufficient for conveying failure.

jingham requested changes to this revision.Oct 4 2016, 2:33 PM
jingham edited edge metadata.

No good deed goes unpunished... You made ParseCanonicalReference more beautiful but forgot to update the header doc.

Also I didn't see the comment for SplitIDRangeExpression.

include/lldb/Breakpoint/BreakpointID.h
75–76

You changed the interface but not the header doc for it.

source/Breakpoint/BreakpointIDList.cpp
350–357

Did you upload the latest version of your patch, I don't see a comment here...

This revision now requires changes to proceed.Oct 4 2016, 2:33 PM
zturner added inline comments.Oct 4 2016, 2:37 PM
source/Breakpoint/BreakpointIDList.cpp
350–357

I put the comment in the header. I can put it here if you prefer. Putting it on both places seems unnecessary though. LMK which you prefer. I'll fix up the header doc of the other function in the meantime.

This revision was automatically updated to reflect the committed changes.