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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
