Greg/Pavel, does the latest revision look good to you? Thanks! On Wed, Apr 18, 2018 at 10:31 AM, Leonard Mosescu via Phabricator < revi...@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 > > > >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
