[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Fixed in 58dd658583eec9af24ca1262e1bce9f884d65487 <https://reviews.llvm.org/rG58dd658583eec9af24ca1262e1bce9f884d65487>

(Also left some nits in the test because I anyway looked over the code.)



================
Comment at: lldb/unittests/Target/FindFileTest.cpp:27
+  FileSpec original;
+  llvm::StringRef remapped;
+  Matches(const char *o, const char *r) : original(o), remapped(r) {}
----------------
I think this can just be a `std::string`. Right now it's relying on the caller to preserve the C-string storage, but that really isn't obvious to the whoever might update the test (and then has to debug a use-after-free).


================
Comment at: lldb/unittests/Target/FindFileTest.cpp:28
+  llvm::StringRef remapped;
+  Matches(const char *o, const char *r) : original(o), remapped(r) {}
+  Matches(const char *o, llvm::sys::path::Style style, const char *r)
----------------
No one is calling this IIUC?


================
Comment at: lldb/unittests/Target/FindFileTest.cpp:29
+  Matches(const char *o, const char *r) : original(o), remapped(r) {}
+  Matches(const char *o, llvm::sys::path::Style style, const char *r)
+      : original(o, style), remapped(r) {}
----------------
I think both of those parameters can be `StringRefs` (FileSpec anyway converts it back to a StringRef).


================
Comment at: lldb/unittests/Target/FindFileTest.cpp:36
+  void SetUp() override {
+    FileSystem::Initialize();
+    HostInfo::Initialize();
----------------
You can simplify this code by giving this test struct a member `SubsystemRAII<FileSystem, HostInfo> subsystems;`. It will automatically call these functions for you in the right order on test setUp/tearDown. It also automatically adds error handling for init errors to your test. So this whole class can all be:

```
lang=c++
struct FindFileTest : public testing::Test {
  SubsystemRAII<FileSystem, HostInfo> subsystems;
};
```


================
Comment at: lldb/unittests/Target/FindFileTest.cpp:58
+
+    EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
+    EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==
----------------
Couldn't `remapped` just be initialized to the FindFile value? Then you don't have to do the whole dance with converting to bool and avoiding the compiler warnings for assignments that look like comparisons.


================
Comment at: lldb/unittests/Target/FindFileTest.cpp:59
+    EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
+    EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==
+                ConstString(match.remapped).GetStringRef());
----------------
`EXPECT_EQ` (StringRef is supported in the gtest macros)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439