[Lldb-commits] [PATCH] D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
This revision was automatically updated to reflect the committed changes.
Closed by commit rL332842: Fix PathMappingList for relative and empty paths
after recent FileSpec… (authored by gclayton, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D47021?vs=147587&id=147780#toc
Repository:
rL LLVM
https://reviews.llvm.org/D47021
Files:
lldb/trunk/include/lldb/Target/PathMappingList.h
lldb/trunk/lldb.xcodeproj/project.pbxproj
lldb/trunk/source/Target/PathMappingList.cpp
lldb/trunk/source/Target/Target.cpp
lldb/trunk/source/Utility/FileSpec.cpp
lldb/trunk/unittests/Target/CMakeLists.txt
lldb/trunk/unittests/Target/PathMappingListTest.cpp
lldb/trunk/unittests/Utility/FileSpecTest.cpp
Index: lldb/trunk/source/Target/Target.cpp
===
--- lldb/trunk/source/Target/Target.cpp
+++ lldb/trunk/source/Target/Target.cpp
@@ -328,11 +328,7 @@
bool hardware,
LazyBool move_to_nearest_code) {
FileSpec remapped_file;
- ConstString remapped_path;
- if (GetSourcePathMap().ReverseRemapPath(ConstString(file.GetPath().c_str()),
- remapped_path))
-remapped_file.SetFile(remapped_path.AsCString(), false);
- else
+ if (!GetSourcePathMap().ReverseRemapPath(file, remapped_file))
remapped_file = file;
if (check_inlines == eLazyBoolCalculate) {
Index: lldb/trunk/source/Target/PathMappingList.cpp
===
--- lldb/trunk/source/Target/PathMappingList.cpp
+++ lldb/trunk/source/Target/PathMappingList.cpp
@@ -14,6 +14,7 @@
// Other libraries and framework includes
// Project includes
+#include "lldb/lldb-private-enumerations.h"
#include "lldb/Host/PosixApi.h"
#include "lldb/Target/PathMappingList.h"
#include "lldb/Utility/FileSpec.h"
@@ -23,6 +24,22 @@
using namespace lldb;
using namespace lldb_private;
+namespace {
+ // We must normalize our path pairs that we store because if we don't then
+ // things won't always work. We found a case where if we did:
+ // (lldb) settings set target.source-map . /tmp
+ // We would store a path pairs of "." and "/tmp" as raw strings. If the debug
+ // info contains "./foo/bar.c", the path will get normalized to "foo/bar.c".
+ // When PathMappingList::RemapPath() is called, it expects the path to start
+ // with the raw path pair, which doesn't work anymore because the paths have
+ // been normalized when the debug info was loaded. So we need to store
+ // nomalized path pairs to ensure things match up.
+ ConstString NormalizePath(const ConstString &path) {
+// If we use "path" to construct a FileSpec, it will normalize the path for
+// us. We then grab the string and turn it back into a ConstString.
+return ConstString(FileSpec(path.GetStringRef(), false).GetPath());
+ }
+}
//--
// PathMappingList constructor
//--
@@ -52,7 +69,7 @@
void PathMappingList::Append(const ConstString &path,
const ConstString &replacement, bool notify) {
++m_mod_id;
- m_pairs.push_back(pair(path, replacement));
+ m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
if (notify && m_callback)
m_callback(*this, m_callback_baton);
}
@@ -77,7 +94,8 @@
insert_iter = m_pairs.end();
else
insert_iter = m_pairs.begin() + index;
- m_pairs.insert(insert_iter, pair(path, replacement));
+ m_pairs.emplace(insert_iter, pair(NormalizePath(path),
+NormalizePath(replacement)));
if (notify && m_callback)
m_callback(*this, m_callback_baton);
}
@@ -88,7 +106,7 @@
if (index >= m_pairs.size())
return false;
++m_mod_id;
- m_pairs[index] = pair(path, replacement);
+ m_pairs[index] = pair(NormalizePath(path), NormalizePath(replacement));
if (notify && m_callback)
m_callback(*this, m_callback_baton);
return true;
@@ -134,58 +152,53 @@
bool PathMappingList::RemapPath(const ConstString &path,
ConstString &new_path) const {
- const char *path_cstr = path.GetCString();
- // CLEANUP: Convert this function to use StringRefs internally instead
- // of raw c-strings.
- if (!path_cstr)
-return false;
-
- const_iterator pos, end = m_pairs.end();
- for (pos = m_pairs.begin(); pos != end; ++pos) {
-const size_t prefixLen = pos->first.GetLength();
-
-if (::strncmp(pos->first.GetCString(), path_cstr, prefixLen) == 0) {
- std::string new_path_str(pos->second.GetCString());
- new_path_str.append(path.GetCString() + prefixLen);
- new_path.SetCString(new_path_str.c_str());
- return true;
-}
+ std::string remapped;
+ if (RemapPath(path.GetStri
[Lldb-commits] [PATCH] D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
looks good. I don't know if anyone else has an opinion on how should we treat
"" for mapping purposes, but treating it as "." seems fine to me.
Comment at: unittests/Utility/FileSpecTest.cpp:300
+ for (const auto &path: not_relative) {
+FileSpec spec(path, false, FileSpec::Style::posix);
+EXPECT_FALSE(spec.IsRelative());
In tests like these, it's good to add something like `SCOPED_TRACE(path)` or
similar. That way, when a test fails you will see which path caused the failure
instead of just a generic "spec.IsRelative() is true" message.
https://reviews.llvm.org/D47021
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
clayborg updated this revision to Diff 147587.
clayborg added a comment.
- Fixed Pavel's issues
- If user specifies "" as the first directory in PathMappingList, it will match
"."
- User can specify "/" as the first directory to remap all absolute paths
- Fixed FileSpec::IsAbsolute() and added tests for issues I discovered when
adding tests for relative paths in PathMappingListTests
- Modified tests in PathMappingListTests to use a help function which
simplifies code
- Added more tests to things that shouldn't get remapped
https://reviews.llvm.org/D47021
Files:
include/lldb/Target/PathMappingList.h
lldb.xcodeproj/project.pbxproj
source/Target/PathMappingList.cpp
source/Target/Target.cpp
source/Utility/FileSpec.cpp
unittests/Target/CMakeLists.txt
unittests/Target/PathMappingListTest.cpp
unittests/Utility/FileSpecTest.cpp
Index: unittests/Utility/FileSpecTest.cpp
===
--- unittests/Utility/FileSpecTest.cpp
+++ unittests/Utility/FileSpecTest.cpp
@@ -273,3 +273,50 @@
EXPECT_EQ("(empty)", llvm::formatv("{0:D}", F).str());
}
+TEST(FileSpecTest, IsRelative) {
+ llvm::StringRef not_relative[] = {
+"/",
+"/a",
+"/a/",
+"/a/b",
+"/a/b/",
+"//",
+"//a",
+"//a/",
+"//a/b",
+"//a/b/",
+"~",
+"~/",
+"~/a",
+"~/a/",
+"~/a/b"
+"~/a/b/",
+"/foo/.",
+"/foo/..",
+"/foo/../",
+"/foo/../.",
+ };
+ for (const auto &path: not_relative) {
+FileSpec spec(path, false, FileSpec::Style::posix);
+EXPECT_FALSE(spec.IsRelative());
+ }
+ llvm::StringRef is_relative[] = {
+".",
+"./",
+".///",
+"a",
+"./a",
+"./a/",
+"./a/",
+"./a/b",
+"./a/b/",
+"../foo",
+"foo/bar.c",
+"./foo/bar.c"
+ };
+ for (const auto &path: is_relative) {
+FileSpec spec(path, false, FileSpec::Style::posix);
+EXPECT_TRUE(spec.IsRelative());
+ }
+}
+
Index: unittests/Target/PathMappingListTest.cpp
===
--- unittests/Target/PathMappingListTest.cpp
+++ unittests/Target/PathMappingListTest.cpp
@@ -0,0 +1,109 @@
+//===-- PathMappingListTest.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "llvm/ADT/ArrayRef.h"
+#include "lldb/Target/PathMappingList.h"
+#include "lldb/Utility/FileSpec.h"
+#include "gtest/gtest.h"
+#include
+
+using namespace lldb_private;
+
+namespace {
+ struct Matches {
+ConstString original;
+ConstString remapped;
+Matches(const char *o, const char *r) : original(o),
+remapped(r) {}
+ };
+
+ void TestPathMappings(const PathMappingList &map,
+llvm::ArrayRef matches,
+llvm::ArrayRef fails) {
+ConstString actual_remapped;
+for (const auto &fail: fails) {
+ EXPECT_FALSE(map.RemapPath(fail, actual_remapped));
+}
+for (const auto &match: matches) {
+ FileSpec orig_spec(match.original.GetStringRef(), false);
+ std::string orig_normalized = orig_spec.GetPath();
+ EXPECT_TRUE(map.RemapPath(match.original, actual_remapped));
+ EXPECT_EQ(actual_remapped, match.remapped);
+ FileSpec remapped_spec(match.remapped.GetStringRef(), false);
+ FileSpec unmapped_spec;
+ EXPECT_TRUE(map.ReverseRemapPath(remapped_spec, unmapped_spec));
+ std::string unmapped_path = unmapped_spec.GetPath();
+ EXPECT_EQ(unmapped_path, orig_normalized);
+}
+ }
+}
+
+TEST(PathMappingListTest, RelativeTests) {
+ Matches matches[] = {
+{".", "/tmp"},
+{"./", "/tmp"},
+{"./", "/tmp"},
+{"./foo.c", "/tmp/foo.c"},
+{"foo.c", "/tmp/foo.c"},
+{"./bar/foo.c", "/tmp/bar/foo.c"},
+{"bar/foo.c", "/tmp/bar/foo.c"},
+ };
+ ConstString fails[] = {
+ConstString("/a"),
+ConstString("/"),
+ };
+ PathMappingList map;
+ map.Append(ConstString("."), ConstString("/tmp"), false);
+ TestPathMappings(map, matches, fails);
+ PathMappingList map2;
+ map2.Append(ConstString(""), ConstString("/tmp"), false);
+ TestPathMappings(map, matches, fails);
+}
+
+TEST(PathMappingListTest, AbsoluteTests) {
+ PathMappingList map;
+ map.Append(ConstString("/old"), ConstString("/new"), false);
+ Matches matches[] = {
+{"/old", "/new"},
+{"/old/", "/new"},
+{"/old/foo/.", "/new/foo"},
+{"/old/foo.c", "/new/foo.c"},
+{"/old/foo.c/.", "/new/foo.c"},
+{"/old/./foo.c", "/new/foo.c"},
+ };
+ ConstString fails[] = {
+ConstString("/foo"),
+ConstString("/"),
+ConstString("foo.c"),
+ConstString("./foo.c"),
+ConstString("../foo.c"),
+ConstString("../bar/foo.c"),
+ };
+ TestPathMappings(map, matches, fails);
+}
+
+TEST(Pa
[Lldb-commits] [PATCH] D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
clayborg added a comment.
I will make the fixes and also test out mapping "" to "." as suggested.
Comment at: source/Target/PathMappingList.cpp:194
+ // path and any path we appended would end up being relative.
+ fixed.SetFile(path_ref, false);
+} else {
When I thought about it, I chose to not convert to "." for path remapping. I
think people would expect if the remap "" to "/prefix" that "/prefix" would be
prepended to each path no matter what it is. Of course users could just specify
"/" if they wish for a prefix. I could see this going either way. Let me know
what you think.
Comment at: unittests/Utility/PathMappingListTest.cpp:86
+{"/old/foo.c/.", "/new/old/foo.c"},
+{"/old/./foo.c", "/new/old/foo.c"},
+ };
labath wrote:
> 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?
It doesn't work. If "foo.c" would map to "/new/foo.c", we can unmap it
correctly since it would unmap to "/foo.c". Both "foo.c" and "/foo.c" would map
to to "/new/foo.c" and then we can only unmap the latter correctly.
This might bode well for saying that "" should map to "." actually. Then we
won't run into this situation. I will test things out with "" mapping to "."
and see how things go.
https://reviews.llvm.org/D47021
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
clayborg marked 7 inline comments as done. clayborg added a comment. Marked things as done. https://reviews.llvm.org/D47021 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
clayborg updated this revision to Diff 147354.
clayborg added a comment.
Fix issues found by Zach.
https://reviews.llvm.org/D47021
Files:
include/lldb/Target/PathMappingList.h
lldb.xcodeproj/project.pbxproj
source/Target/PathMappingList.cpp
source/Target/Target.cpp
unittests/Utility/CMakeLists.txt
unittests/Utility/PathMappingListTest.cpp
Index: unittests/Utility/PathMappingListTest.cpp
===
--- unittests/Utility/PathMappingListTest.cpp
+++ unittests/Utility/PathMappingListTest.cpp
@@ -0,0 +1,101 @@
+//===-- NameMatchesTest.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Target/PathMappingList.h"
+#include "lldb/Utility/FileSpec.h"
+#include "gtest/gtest.h"
+#include
+
+using namespace lldb_private;
+
+namespace {
+ struct Matches {
+ConstString original;
+ConstString remapped;
+Matches(const char *o, const char *r) : original(o),
+remapped(r) {}
+ };
+}
+
+TEST(PathMappingListTest, RelativeTests) {
+ PathMappingList map;
+ map.Append(ConstString("."), ConstString("/tmp"), false);
+ Matches tests[] = {
+{".", "/tmp"},
+{"./", "/tmp"},
+{"./", "/tmp"},
+{"./foo.c", "/tmp/foo.c"},
+{"./bar/foo.c", "/tmp/bar/foo.c"}
+ };
+ ConstString actual_remapped;
+ EXPECT_FALSE(map.RemapPath(ConstString("/foo"), actual_remapped));
+ for (const auto &m: tests) {
+FileSpec orig_spec(m.original.GetCString(), false);
+std::string orig_normalized = orig_spec.GetPath();
+EXPECT_TRUE(map.RemapPath(m.original, actual_remapped));
+EXPECT_TRUE(actual_remapped == m.remapped);
+FileSpec remapped_spec(m.remapped.GetCString(), false);
+FileSpec unmapped_spec;
+EXPECT_TRUE(map.ReverseRemapPath(remapped_spec, unmapped_spec));
+std::string unmapped_path = unmapped_spec.GetPath();
+EXPECT_TRUE(unmapped_path == orig_normalized);
+ }
+}
+
+TEST(PathMappingListTest, AbsoluteTests) {
+ PathMappingList map;
+ map.Append(ConstString("/old"), ConstString("/new"), false);
+ Matches tests[] = {
+{"/old", "/new"},
+{"/old/", "/new"},
+{"/old/foo/.", "/new/foo"},
+{"/old/foo.c", "/new/foo.c"},
+{"/old/foo.c/.", "/new/foo.c"},
+{"/old/./foo.c", "/new/foo.c"},
+ };
+ ConstString actual_remapped;
+ // Make sure that the path
+ for (const auto &m: tests) {
+FileSpec orig_spec(m.original.GetCString(), false);
+std::string orig_normalized = orig_spec.GetPath();
+EXPECT_TRUE(map.RemapPath(m.original, actual_remapped));
+EXPECT_TRUE(actual_remapped == m.remapped);
+FileSpec remapped_spec(m.remapped.GetCString(), false);
+FileSpec unmapped_spec;
+EXPECT_TRUE(map.ReverseRemapPath(remapped_spec, unmapped_spec));
+std::string unmapped_path = unmapped_spec.GetPath();
+EXPECT_TRUE(unmapped_path == orig_normalized);
+ }
+}
+
+TEST(PathMappingListTest, RemapEverything) {
+ PathMappingList map;
+ map.Append(ConstString(""), ConstString("/new"), false);
+ Matches tests[] = {
+{"/old", "/new/old"},
+{"/old/", "/new/old"},
+{"/old/foo/.", "/new/old/foo"},
+{"/old/foo.c", "/new/old/foo.c"},
+{"/old/foo.c/.", "/new/old/foo.c"},
+{"/old/./foo.c", "/new/old/foo.c"},
+ };
+ ConstString actual_remapped;
+ // Make sure that the path
+ for (const auto &m: tests) {
+FileSpec orig_spec(m.original.GetCString(), false);
+std::string orig_normalized = orig_spec.GetPath();
+EXPECT_TRUE(map.RemapPath(m.original, actual_remapped));
+EXPECT_TRUE(actual_remapped == m.remapped);
+FileSpec remapped_spec(m.remapped.GetCString(), false);
+FileSpec unmapped_spec;
+EXPECT_TRUE(map.ReverseRemapPath(remapped_spec, unmapped_spec));
+std::string unmapped_path = unmapped_spec.GetPath();
+EXPECT_TRUE(unmapped_path == orig_normalized);
+ }
+}
Index: unittests/Utility/CMakeLists.txt
===
--- unittests/Utility/CMakeLists.txt
+++ unittests/Utility/CMakeLists.txt
@@ -8,6 +8,7 @@
JSONTest.cpp
LogTest.cpp
NameMatchesTest.cpp
+ PathMappingListTest.cpp
StatusTest.cpp
StringExtractorTest.cpp
StructuredDataTest.cpp
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -328,11 +328,7 @@
bool hardware,
LazyBool move_to_nearest_code) {
FileSpec remapped_file;
- ConstString remapped_path;
- if (GetSourcePathMap().ReverseRemapPath(ConstString(file.GetPath().c_str()),
- remapped_path))
-remapped_file.SetFile(remapp
[Lldb-commits] [PATCH] D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
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
[Lldb-commits] [PATCH] D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
clayborg created this revision.
clayborg added reviewers: labath, zturner, davide.
Herald added subscribers: JDevlieghere, aprantl, mgorny.
PathMappingList was broken for relative and empty paths after normalization
changes in FileSpec. There were also no tests for PathMappingList so I added
those.
Changes include:
- Change PathMappingList::ReverseRemapPath() to take FileSpec objects instead
of ConstString. The only client of this was doing work to convert to and from
ConstString objects for no reason.
- Normalize all paths prefix and replacements that are added to the
PathMappingList vector so they match the paths that have been already
normalized in the debug info
- Unify code in the two forms of PathMappingList::RemapPath() so only one
contains the actual functionality. Prior to this, there were two versions of
this code.
- Use FileSpec::AppendPathComponent() and remove a long standing TODO so paths
are correctly appended to each other.
- Correctly handle the case where someone maps "" to something else. This
allows all paths to be prefixed with the replacement.
- Added tests for absolute, relative and empty paths.
https://reviews.llvm.org/D47021
Files:
include/lldb/Target/PathMappingList.h
lldb.xcodeproj/project.pbxproj
source/Target/PathMappingList.cpp
source/Target/Target.cpp
unittests/Utility/CMakeLists.txt
unittests/Utility/PathMappingListTest.cpp
Index: unittests/Utility/PathMappingListTest.cpp
===
--- unittests/Utility/PathMappingListTest.cpp
+++ unittests/Utility/PathMappingListTest.cpp
@@ -0,0 +1,101 @@
+//===-- NameMatchesTest.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Target/PathMappingList.h"
+#include "lldb/Utility/FileSpec.h"
+#include "gtest/gtest.h"
+#include
+
+using namespace lldb_private;
+
+namespace {
+ struct Matches {
+ConstString original;
+ConstString remapped;
+Matches(const char *o, const char *r) : original(o),
+remapped(r) {}
+ };
+}
+
+TEST(PathMappingListTest, RelativeTests) {
+ PathMappingList map;
+ map.Append(ConstString("."), ConstString("/tmp"), false);
+ Matches tests[] = {
+{".", "/tmp"},
+{"./", "/tmp"},
+{"./", "/tmp"},
+{"./foo.c", "/tmp/foo.c"},
+{"./bar/foo.c", "/tmp/bar/foo.c"}
+ };
+ ConstString actual_remapped;
+ EXPECT_FALSE(map.RemapPath(ConstString("/foo"), actual_remapped));
+ for (const auto &m: tests) {
+FileSpec orig_spec(m.original.GetCString(), false);
+std::string orig_normalized = orig_spec.GetPath();
+EXPECT_TRUE(map.RemapPath(m.original, actual_remapped));
+EXPECT_TRUE(actual_remapped == m.remapped);
+FileSpec remapped_spec(m.remapped.GetCString(), false);
+FileSpec unmapped_spec;
+EXPECT_TRUE(map.ReverseRemapPath(remapped_spec, unmapped_spec));
+std::string unmapped_path = unmapped_spec.GetPath();
+EXPECT_TRUE(unmapped_path == orig_normalized);
+ }
+}
+
+TEST(PathMappingListTest, AbsoluteTests) {
+ PathMappingList map;
+ map.Append(ConstString("/old"), ConstString("/new"), false);
+ Matches tests[] = {
+{"/old", "/new"},
+{"/old/", "/new"},
+{"/old/foo/.", "/new/foo"},
+{"/old/foo.c", "/new/foo.c"},
+{"/old/foo.c/.", "/new/foo.c"},
+{"/old/./foo.c", "/new/foo.c"},
+ };
+ ConstString actual_remapped;
+ // Make sure that the path
+ for (const auto &m: tests) {
+FileSpec orig_spec(m.original.GetCString(), false);
+std::string orig_normalized = orig_spec.GetPath();
+EXPECT_TRUE(map.RemapPath(m.original, actual_remapped));
+EXPECT_TRUE(actual_remapped == m.remapped);
+FileSpec remapped_spec(m.remapped.GetCString(), false);
+FileSpec unmapped_spec;
+EXPECT_TRUE(map.ReverseRemapPath(remapped_spec, unmapped_spec));
+std::string unmapped_path = unmapped_spec.GetPath();
+EXPECT_TRUE(unmapped_path == orig_normalized);
+ }
+}
+
+TEST(PathMappingListTest, RemapEverything) {
+ PathMappingList map;
+ map.Append(ConstString(""), ConstString("/new"), false);
+ Matches tests[] = {
+{"/old", "/new/old"},
+{"/old/", "/new/old"},
+{"/old/foo/.", "/new/old/foo"},
+{"/old/foo.c", "/new/old/foo.c"},
+{"/old/foo.c/.", "/new/old/foo.c"},
+{"/old/./foo.c", "/new/old/foo.c"},
+ };
+ ConstString actual_remapped;
+ // Make sure that the path
+ for (const auto &m: tests) {
+FileSpec orig_spec(m.original.GetCString(), false);
+std::string orig_normalized = orig_spec.GetPath();
+EXPECT_TRUE(map.RemapPath(m.original, actual_remapped));
+EXPECT_TRUE(actual_remapped == m.remapped);
+FileSpec remapped_spec(m.remapped.GetCString(), false);
+FileSpec unmapped_spec;
+EXPECT_TRUE(map.Re
