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

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: source/Target/PathMappingList.cpp:76
   ++m_mod_id;
-  m_pairs.push_back(pair(path, replacement));
+  m_pairs.push_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
----------------
Slightly more idiomatic to say `emplace_back(NormalizePath(path), NormalizePath(replacement));`


================
Comment at: source/Target/PathMappingList.cpp:178
+    remapped.AppendPathComponent(path);
+    new_path = std::move(remapped.GetPath());
     return true;
----------------
I don't think the `std::move` is necessary here.  Since the result of `GetPath()` is already a temporary (rvalue), the move assignment operator will already be invoked even without `std::move`.


================
Comment at: source/Target/PathMappingList.cpp:186
+  std::string path = file.GetPath();
+  llvm::StringRef path_ref(path.data(), path.size());
   for (const auto &it : m_pairs) {
----------------
`llvm::StringRef path_ref(path);` should be fine, no need to explicitly specify the pointer and size.


================
Comment at: source/Target/PathMappingList.cpp:188
   for (const auto &it : m_pairs) {
-    // FIXME: This should be using FileSpec API's to do the path appending.
-    const size_t prefixLen = it.second.GetLength();
-    if (::strncmp(it.second.GetCString(), path_cstr, prefixLen) == 0) {
-      std::string new_path_str(it.first.GetCString());
-      new_path_str.append(path.GetCString() + prefixLen);
-      new_path.SetCString(new_path_str.c_str());
+    llvm::StringRef second(it.second.GetCString(), it.second.GetLength());
+    if (path_ref.startswith(second)) {
----------------
How about `llvm::StringRef second = it.second.GetStringRef();`


================
Comment at: source/Target/PathMappingList.cpp:189
+    llvm::StringRef second(it.second.GetCString(), it.second.GetLength());
+    if (path_ref.startswith(second)) {
+      llvm::StringRef first(it.first.GetCString(), it.first.GetLength());
----------------
Can you invert this conditional and `continue;` if it's false?


================
Comment at: source/Target/PathMappingList.cpp:189
+    llvm::StringRef second(it.second.GetCString(), it.second.GetLength());
+    if (path_ref.startswith(second)) {
+      llvm::StringRef first(it.first.GetCString(), it.first.GetLength());
----------------
zturner wrote:
> Can you invert this conditional and `continue;` if it's false?
It looks like after this check, the first `second.size()` characters of `path_ref` are ignored.  So it sounds like you can write this as:

```
if (!path_ref.consume_front(second))
  continue;
```

Then further down you can delete the line that calls `substr`.


================
Comment at: source/Target/PathMappingList.cpp:190
+    if (path_ref.startswith(second)) {
+      llvm::StringRef first(it.first.GetCString(), it.first.GetLength());
+      llvm::StringRef suffix = path_ref.substr(second.size());
----------------
`llvm::StringRef first = it.first.GetStringRef();`


https://reviews.llvm.org/D47021