[Lldb-commits] [PATCH] D76805: Fix SourceManager::SourceFileCache insertion

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath marked an inline comment as done.
labath added a comment.
This revision is now accepted and ready to land.

Looks good. I'm just picking some nits in the test. I'm assuming that @jingham's comment refers to the other patch (and it more-or-less matches what I wrote there already).

Does this actually depend on the other patch? It looks like an independent fix we could commit separately.



================
Comment at: lldb/source/Core/SourceManager.cpp:699
 void SourceManager::SourceFileCache::AddSourceFile(const FileSP &file_sp) {
-  FileSpec file_spec;
   FileCache::iterator pos = m_file_cache.find(file_spec);
----------------
Woops :)


================
Comment at: lldb/unittests/Core/SourceManagerTest.cpp:11
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Utility/RegularExpression.h"
+
----------------
This doesn't appear to be needed.


================
Comment at: lldb/unittests/Core/SourceManagerTest.cpp:24-31
+TEST_F(SourceFileCache, AddSourceFile) {
+  SourceManager::SourceFileCache cache;
+
+  // Insert: foo
+  FileSpec file_spec("foo");
+  auto file_sp = std::make_shared<SourceManager::File>(file_spec, nullptr);
+  cache.AddSourceFile(file_sp);
----------------
A test without any assertion is weird. I'd recommend just deleting this since insertion is already tested in the other tests.


================
Comment at: lldb/unittests/Core/SourceManagerTest.cpp:60-63
+  SourceManager::FileSP result = cache.FindSourceFile(bar_file_spec);
+
+  // Expect not found.
+  ASSERT_EQ(result, nullptr);
----------------
I'd recommend folding these two statements into one (ASSERT_EQ(cache.FindSourceFile(...), nullptr)). That way we'll have at least a semi-reasonable error message when this fails instead of a "0xdeadbeef != 0"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76805