[Lldb-commits] [PATCH] D76806: Remove m_last_file_sp from SourceManager

[Pavel Labath via Phabricator via lldb-commits]

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