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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
