[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked an inline comment as done.
lemo added a comment.

In https://reviews.llvm.org/D45700#1070491, @amccarth wrote:

> LGTM, but consider highlighting the side effect to `target` when the factory makes a Placeholder module.


Good observation: taking a step back, the factory introduces too much coupling, especially if we want to extend this placeholder module approach to other uses. Because of this, I reverted back to the standalone PlaceholderModule::CreateImageSection() approach. Thanks Adrian!



================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:70
+  // Creates a synthetic module section covering the whole module image
+  void CreateImageSection(const MinidumpModule *module, Target& target) {
+    const ConstString section_name(".module_image");
----------------
amccarth wrote:
> I didn't notice before that target is a non-const ref.  Maybe the comment should explain why target is modified (and/or maybe in PlaceholderModule::Create).
Updated the function comment. This is similar to how other places set the section load address (ex. ObjectFileELF::SetLoadAddress)


https://reviews.llvm.org/D45700