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

[Leonard Mosescu via lldb-commits]

Greg/Pavel, does the latest revision look good to you? Thanks!

On Wed, Apr 18, 2018 at 10:31 AM, Leonard Mosescu via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180418/5e0cb61f/attachment.html>