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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
