[Lldb-commits] [PATCH] D95686: [lldb/API] Expose Module::IsLoadedInTarget() to SB API (NFC)

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Wouldn't it make more sense to give `SBTarget` a `IsLoaded` function? The function reads more like "please check if this target is loaded" which doesn't make sense. I get that this is how the internal API is doing it, but as we can't change the SB API I would rather have this to be logical from the start.

Also this should have a Python doc string. The short explanation what "loaded" means on the internal function is probably good enough. Something like this (assuming this will be an SBTarget function):

  Returns true if the module has been loaded in this `SBTarget`.
  
  A module can be loaded either by the dynamic loader or by being manually
  added to the target (see `SBTarget.AddModule` and the `target module add` command).



================
Comment at: lldb/test/API/python_api/module_section/TestModuleAndSection.py:179
+                                                          line_number(self.main_source_file.GetFilename(),
+                                                                      'Hello'))
+
----------------
Out of curiosity: Do we need to start the program to load all modules? I thought self.dbg.CreateTarget() would be enough (which would prevent this test from requiring a process start).


================
Comment at: lldb/test/API/python_api/module_section/TestModuleAndSection.py:186
+            self.assertTrue(module_is_loaded, "Module is not loaded in "
+                            "target.")
----------------
I think one single assert that checks for an invalid passed argument would be nice. Also I guess calling this on the dummy target and making sure that it doesn't have any of the real target's modules loaded would be nice (right now `return true;` would be a valid implementation for this function that passes all tests).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95686