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

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Nice!



================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:53
+  // Creates a synthetic module section covering the whole module image
+  void CreateImageSection(const MinidumpModule *module, Target& target) {
+    const ConstString section_name(".module_image");
----------------
I wonder if this should just be part of the constructor.  I don't see a scenario where you'd create a PlaceholderModule and not want to create exactly one fake image section.  I know the style guide is against doing lots of work in a constructor, but that's mostly because it's hard to report failures, which you're not doing here anyway.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:329
     if (!module_sp || error.Fail()) {
-      continue;
+      // We failed to locate a matching local object file. Fortunatelly
+      // the minidump format encodes enough information about each module's
----------------
nit:  s/Fortunatelly/Fortunately/


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:339
+      my_module->CreateImageSection(module, GetTarget());
+      module_sp = my_module;
+      GetTarget().GetImages().Append(module_sp);
----------------
If CreateImageSection bit were done by the PlaceholderModule's constructor, then there'd be no need for the artificial `my_module` temporary, and nobody would ever make a PlaceholderModule without an image section.


https://reviews.llvm.org/D45700