labath added inline comments.
================ Comment at: lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py:17 + + @skipIf(oslist=no_match(["windows"])) + def test_set_use_source_cache_false(self): ---------------- Could you remove this decorator? The test is not extremely interesting on non-windows hosts, but the flow should still work, so there's no harm in testing it. ================ Comment at: lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py:22 + + @skipIf(oslist=no_match(["windows"])) + def test_set_use_source_cache_true(self): ---------------- technically, this should be `hostoslist=no_match(...)` ================ Comment at: lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py:30-70 + self.build() + exe = self.getBuildArtifact("a.out") + self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET) + + # Enable/Disable source cache + self.runCmd( + "settings set use-source-cache " + ---------------- All of this could be done via a single `lldbutil.run_to_source_breakpoint` call (that's a fairly new thing, so it's possible the test you based this on is not using it yet). ================ Comment at: lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py:40 + # Get paths for source files: header.h, header2.h + src_file = self.getSourcePath("header.h") + self.assertTrue(src_file) ---------------- Please avoid modifying the source tree. You can take a look at `test/API/source-manager/Makefile` to see how to build binaries with sources in the build tree. ================ Comment at: lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py:86-87 + + if is_cache_enabled and is_file_removed: + raise Exception("Source cache is enabled, but delete file succeeded") + ---------------- maybe: ``` if is_cache_enabled: self.assertFalse(is_file_removed) ``` (and similarly for the other case). ================ Comment at: lldb/test/API/commands/settings/use_source_cache/header.h:1 +// This file should be large enough that LLDB decides to load it +// using memory mapping. See: ---------------- Does this need to be in a separate file? Could you just put it into main.cpp ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76806/new/ https://reviews.llvm.org/D76806 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits