[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda marked 2 inline comments as done.
jasonmolenda added a comment.

Thanks for looking the patch over.



================
Comment at: include/lldb/Target/Target.h:535
+                           bool notify,
+                           Status *error_ptr = nullptr);
 
----------------
clayborg wrote:
> Pavel had questions about this error. If we specify an error when we call this, is there a way to get a valid module shared pointer back and still get an error? Maybe this should be one of the llvm::ErrorOr return types?
There's only a handful of AddModule callers that pass in a Status object - the Windows ones, the CommandObjectTarget command, and the Minidump.  I'll look through the Platform GetSharedModules implementations tomorrow to see who might put useful things in the error object, but in general either AddModule finds a binary that matches the ModuleSpec, and returns a shared pointer to it, or finds nothing and returns an empty shared pointer.  The error object may have a message from a Platform indicating why the search failed ("could not find any SDK directories" or something) but I suspect this optional Status arg isn't doing anything.


================
Comment at: source/Commands/CommandObjectTarget.cpp:399-400
+          const bool notify = true;
+          ModuleSP module_sp = target_sp->AddModule(main_module_spec,
+                                                          notify);
           if (module_sp)
----------------
clayborg wrote:
> Remove all "const bool notify = true; "statements and Inline with comment?
> 
> ```
> ModuleSP module_sp = target_sp->AddModule(main_module_spec, 
>                                           true /*notify*/);
> ```
> This would apply everywhere in this patch if so.
Can do.  I don't have a preference.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60172/new/

https://reviews.llvm.org/D60172