This is an archive of the discontinued LLVM Phabricator instance.

Use std::make_shared in LLDB (NFC)
ClosedPublic

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

Details

Summary

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

Repository
rLLDB LLDB

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.
lldb/source/API/SBData.cpp
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
lldb/source/API/SBData.cpp
12 ↗(On Diff #186086)

How?

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.
lldb/source/API/SBData.cpp
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
lldb/source/API/SBData.cpp
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:

lldb/source/API/SBInstruction.cpp
lldb/source/API/SBThread.cpp
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Interpreter/CommandInterpreter.cpp

JDevlieghere marked an inline comment as done.Feb 8 2019, 7:10 PM
JDevlieghere added inline comments.
lldb/source/API/SBData.cpp
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
lldb/source/API/SBData.cpp
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:

lldb/source/API/SBCommandInterpreter.cpp
560 ↗(On Diff #186086)

Why not replace assignment with initialization here and below?

lldb/source/Target/Thread.cpp
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:

lldb/source/API/SBTypeEnumMember.cpp
78 ↗(On Diff #186086)
if (!m_opaque_sp)
lldb/source/Core/IOHandler.cpp
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
lldb/source/API/SBTypeEnumMember.cpp
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?

lldb/source/API/SBData.cpp
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
lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
10 ↗(On Diff #186279)

Spurious separator.

lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp
10 ↗(On Diff #186279)

Spurious separator.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
11 ↗(On Diff #186279)

Wrong place.

lldb/tools/debugserver/source/debugserver.cpp
14 ↗(On Diff #186279)

Wrong place.

This revision was automatically updated to reflect the committed changes.