Page MenuHomePhabricator

Use std::make_shared in LLDB (NFC)

Authored by JDevlieghere on Feb 8 2019, 5:42 PM.



Unlike std::make_unique, which is only available since C++14,
std::make_shared is available since C++11. Not only is std::make_shared
a lot more readable compared to ::reset(new), it also performs a single
heap allocation for the object and control block.

Diff Detail


Event Timeline

JDevlieghere created this revision.Feb 8 2019, 5:42 PM
Herald added a project: Restricted Project. · View Herald Transcript
Eugene.Zelenko added inline comments.
9 ↗(On Diff #186086)

cinttypes should be used instead. See Clang-tidy modernize-deprecated-headers. Same in other places.

12 ↗(On Diff #186086)

Spaces between include statements interfere with Clang-format. Same in other places.

jingham added inline comments.Feb 8 2019, 6:12 PM
12 ↗(On Diff #186086)


We always separate system header includes from lldb includes from llvm includes by putting a blank line. That's done all over the place in lldb. I haven't heard of this causing problems before now. If this does cause problems, that should be fixed in clang-format. It seems like a really unreasonable requirement.

JDevlieghere marked an inline comment as done.Feb 8 2019, 6:13 PM
JDevlieghere added inline comments.
12 ↗(On Diff #186086)

Indeed, and as far as I know llvm does exactly the same thing. clang-format sorts headers within the same group, which is exactly what we want.

Eugene.Zelenko added inline comments.Feb 8 2019, 6:50 PM
12 ↗(On Diff #186086)

Is there any good reason to be different from LLVM/Clang? Wasn't formatting style was changed in not so recent past to reduce differences?

Examples of problems visible in this patch:


JDevlieghere marked an inline comment as done.Feb 8 2019, 7:10 PM
JDevlieghere added inline comments.
12 ↗(On Diff #186086)

I'm saying it's exactly the same in LLDB as it is in LLVM. I'm afraid I don't understand what you mean here.

Eugene.Zelenko added inline comments.Feb 8 2019, 7:16 PM
12 ↗(On Diff #186086)

I don't remember empty lines between headers groups in LLVM and Clang code base.

jfb added a comment.Feb 8 2019, 9:10 PM

Do you ever use weak pointers? The single allocation is good until you start holding on to more things (because you can't just keep the block around, you need the entire allocation), so you should make sure you won’t be delaying memory being reclaimed.

I agree with Eugene that the empty lines interfere with clang-format and that llvm does not use empty lines between system and own headers. However, our headers are already very inconsistent with that, so I don't think fixing that should be a part of this patch. It would be good if this patch did not make the problem worse though, which in some cases it does: e.g. in SBInstruction.cpp, <memory> is inserted into a middle of a SB include group together with an empty line, which prevents clang-format from fixing this up.

Apart from that, this lgtm. Lldb does use weak_ptrs in a bunch of places, but I'm not terribly worried about the memory impact of this change.

A few modernizations which though don't directly relate to make_shared:

560 ↗(On Diff #186086)

Why not replace assignment with initialization here and below?

1611 ↗(On Diff #186086)

This 'if-else' block may be replaced with

if (!m_curr_frames_sp)
  m_curr_frames_sp =
      std::make_shared<StackFrameList>(*this, m_prev_frames_sp, true);
frame_list_sp = m_curr_frames_sp;

These also don't relate directly but intersect with your changes:

78 ↗(On Diff #186086)
if (!m_opaque_sp)
1012 ↗(On Diff #186086)

Also may be simplified

auto get_window = [this, &bounds] () {
  return m_window ?
      ::subwin(m_window, bounds.size.height, bounds.size.width,
                   bounds.origin.y, bounds.origin.x) :
      ::newwin(bounds.size.height, bounds.size.width,
                   bounds.origin.y, bounds.origin.x);

subwindow_sp = std::make_shared<Window>(name, get_window(), true);
subwindow_sp->m_is_subwin = m_window.operator bool();
Eugene.Zelenko added inline comments.Feb 11 2019, 7:55 AM
78 ↗(On Diff #186086)

Will be good idea to run Clang-tidy readability-redundant-smartptr-get.

JDevlieghere marked an inline comment as done.
  • Fix <memory> include.

@tatyana-krasnukha and @Eugene.Zelenko, are you okay with me addressing those thing in a follow-up?

12 ↗(On Diff #186086)

I misunderstood, you're correct!

davide accepted this revision.Feb 11 2019, 2:34 PM
This revision is now accepted and ready to land.Feb 11 2019, 2:34 PM
Eugene.Zelenko added inline comments.Feb 11 2019, 2:37 PM
10 ↗(On Diff #186279)

Spurious separator.

10 ↗(On Diff #186279)

Spurious separator.

11 ↗(On Diff #186279)

Wrong place.

14 ↗(On Diff #186279)

Wrong place.

This revision was automatically updated to reflect the committed changes.