[Lldb-commits] [PATCH] D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Although it may not seem that way from the number of comments, the change looks good to me. The main thing is the moving of the test file, as that will fail in the cmake build. And it also looks like some code can be simplified if my assumption about not converting "" to "." is true.

Thank you for adding the test for this class.



================
Comment at: source/Target/PathMappingList.cpp:44
+    // us. We then grab the string and turn it back into a ConstString.
+    return ConstString(FileSpec(path.GetCString(), false).GetPath());
+  }
----------------
s/GetCString/GetStringRef


================
Comment at: source/Target/PathMappingList.cpp:101
     insert_iter = m_pairs.begin() + index;
-  m_pairs.insert(insert_iter, pair(path, replacement));
+  m_pairs.insert(insert_iter, pair(NormalizePath(path),
+                                   NormalizePath(replacement)));
----------------
s/insert/emplace


================
Comment at: source/Target/PathMappingList.cpp:161
+  if (RemapPath(path.GetStringRef(), remapped)) {
+    new_path.SetCStringWithLength(remapped.c_str(), remapped.size());
+    return true;
----------------
SetString(remapped)


================
Comment at: source/Target/PathMappingList.cpp:191-194
+      // If our original path was "", then the new path is just the suffix.
+      // If we call fixed.SetFile("", false) we will end up with "." as the
+      // path and any path we appended would end up being relative.
+      fixed.SetFile(path_ref, false);
----------------
Is this really true? Judging by the test you've had to remove in r332633, we shouldn't end up converting "" to ".".


================
Comment at: unittests/Utility/PathMappingListTest.cpp:1
+//===-- NameMatchesTest.cpp -------------------------------------*- C++ -*-===//
+//
----------------
This file should go to `unittests/Target/PathMappingListTest.cpp` to match where the class-under-test lives. (Also, please update the name in the header.)


================
Comment at: unittests/Utility/PathMappingListTest.cpp:39
+  for (const auto &m: tests) {
+    FileSpec orig_spec(m.original.GetCString(), false);
+    std::string orig_normalized = orig_spec.GetPath();
----------------
`s/GetCString/GetStringRef` (whole file)


================
Comment at: unittests/Utility/PathMappingListTest.cpp:42
+    EXPECT_TRUE(map.RemapPath(m.original, actual_remapped));
+    EXPECT_TRUE(actual_remapped == m.remapped);
+    FileSpec remapped_spec(m.remapped.GetCString(), false);
----------------
It's better to use `EXPECT_EQ(expected, actual)` for equality comparison, as that will print out a more useful error message when things fail.


================
Comment at: unittests/Utility/PathMappingListTest.cpp:86
+    {"/old/foo.c/.", "/new/old/foo.c"},
+    {"/old/./foo.c", "/new/old/foo.c"},
+  };
----------------
How does this work for relative paths? I take it `foo.c` should be remapped to `/new/foo.c` ? Can you add a test for that?


https://reviews.llvm.org/D47021