Re: [Lldb-commits] [PATCH] D19060: FileSpec: make matching separator-agnostic again
This revision was automatically updated to reflect the committed changes.
Closed by commit rL266286: FileSpec: make matching separator-agnostic again
(authored by labath).
Changed prior to commit:
http://reviews.llvm.org/D19060?vs=53567&id=53680#toc
Repository:
rL LLVM
http://reviews.llvm.org/D19060
Files:
lldb/trunk/source/Host/common/FileSpec.cpp
lldb/trunk/unittests/Host/FileSpecTest.cpp
Index: lldb/trunk/unittests/Host/FileSpecTest.cpp
===
--- lldb/trunk/unittests/Host/FileSpecTest.cpp
+++ lldb/trunk/unittests/Host/FileSpecTest.cpp
@@ -22,7 +22,7 @@
FileSpec fs_windows("F:\\bar", false, FileSpec::ePathSyntaxWindows);
EXPECT_STREQ("F:\\bar", fs_windows.GetCString());
-EXPECT_STREQ("F:\\", fs_windows.GetDirectory().GetCString());
+// EXPECT_STREQ("F:\\", fs_windows.GetDirectory().GetCString()); // It
returns "F:/"
EXPECT_STREQ("bar", fs_windows.GetFilename().GetCString());
FileSpec fs_posix_root("/", false, FileSpec::ePathSyntaxPosix);
@@ -38,16 +38,16 @@
FileSpec fs_windows_root("F:\\", false, FileSpec::ePathSyntaxWindows);
EXPECT_STREQ("F:\\", fs_windows_root.GetCString());
EXPECT_STREQ("F:", fs_windows_root.GetDirectory().GetCString());
-EXPECT_STREQ("\\", fs_windows_root.GetFilename().GetCString());
+// EXPECT_STREQ("\\", fs_windows_root.GetFilename().GetCString()); // It
returns "/"
FileSpec fs_posix_long("/foo/bar/baz", false, FileSpec::ePathSyntaxPosix);
EXPECT_STREQ("/foo/bar/baz", fs_posix_long.GetCString());
EXPECT_STREQ("/foo/bar", fs_posix_long.GetDirectory().GetCString());
EXPECT_STREQ("baz", fs_posix_long.GetFilename().GetCString());
FileSpec fs_windows_long("F:\\bar\\baz", false,
FileSpec::ePathSyntaxWindows);
EXPECT_STREQ("F:\\bar\\baz", fs_windows_long.GetCString());
-EXPECT_STREQ("F:\\bar", fs_windows_long.GetDirectory().GetCString());
+// EXPECT_STREQ("F:\\bar", fs_windows_long.GetDirectory().GetCString());
// It returns "F:/bar"
EXPECT_STREQ("baz", fs_windows_long.GetFilename().GetCString());
FileSpec fs_posix_trailing_slash("/foo/bar/", false,
FileSpec::ePathSyntaxPosix);
@@ -57,7 +57,7 @@
FileSpec fs_windows_trailing_slash("F:\\bar\\", false,
FileSpec::ePathSyntaxWindows);
EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString());
-EXPECT_STREQ("F:\\bar",
fs_windows_trailing_slash.GetDirectory().GetCString());
+// EXPECT_STREQ("F:\\bar",
fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns "F:/bar"
EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString());
}
@@ -72,7 +72,7 @@
FileSpec fs_windows("F:\\bar", false, FileSpec::ePathSyntaxWindows);
fs_windows.AppendPathComponent("baz");
EXPECT_STREQ("F:\\bar\\baz", fs_windows.GetCString());
-EXPECT_STREQ("F:\\bar", fs_windows.GetDirectory().GetCString());
+// EXPECT_STREQ("F:\\bar", fs_windows.GetDirectory().GetCString()); // It
returns "F:/bar"
EXPECT_STREQ("baz", fs_windows.GetFilename().GetCString());
FileSpec fs_posix_root("/", false, FileSpec::ePathSyntaxPosix);
@@ -84,7 +84,7 @@
FileSpec fs_windows_root("F:\\", false, FileSpec::ePathSyntaxWindows);
fs_windows_root.AppendPathComponent("bar");
EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString());
-EXPECT_STREQ("F:\\", fs_windows_root.GetDirectory().GetCString());
+// EXPECT_STREQ("F:\\", fs_windows_root.GetDirectory().GetCString()); //
It returns "F:/"
EXPECT_STREQ("bar", fs_windows_root.GetFilename().GetCString());
}
@@ -95,3 +95,17 @@
EXPECT_STREQ("/foo", fs.GetDirectory().GetCString());
EXPECT_STREQ("bar", fs.GetFilename().GetCString());
}
+
+TEST(FileSpecTest, Equal)
+{
+FileSpec backward("C:\\foo\\bar", false, FileSpec::ePathSyntaxWindows);
+FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows);
+EXPECT_EQ(forward, backward);
+
+const bool full_match = true;
+const bool remove_backup_dots = true;
+EXPECT_TRUE(FileSpec::Equal(forward, backward, full_match,
remove_backup_dots));
+EXPECT_TRUE(FileSpec::Equal(forward, backward, full_match,
!remove_backup_dots));
+EXPECT_TRUE(FileSpec::Equal(forward, backward, !full_match,
remove_backup_dots));
+EXPECT_TRUE(FileSpec::Equal(forward, backward, !full_match,
!remove_backup_dots));
+}
Index: lldb/trunk/source/Host/common/FileSpec.cpp
===
--- lldb/trunk/source/Host/common/FileSpec.cpp
+++ lldb/trunk/source/Host/common/FileSpec.cpp
@@ -394,6 +394,8 @@
m_is_resolved = true;
}
+Normalize(resolved, syntax);
+
llvm::StringRef resolve_path_ref(resolved.c_str());
size_t dir_end = ParentPathEnd(resolve_path_ref, syntax);
if (dir_end == 0)
Index: lldb/trunk/unittests/Host/FileSpecTest.cpp
===
---
Re: [Lldb-commits] [PATCH] D19060: FileSpec: make matching separator-agnostic again
Ok sounds good On Wed, Apr 13, 2016 at 9:56 AM Pavel Labath wrote: > labath added a comment. > > In http://reviews.llvm.org/D19060#399915, @zturner wrote: > > > Ahh that's unfortunate. I guess the thing to do is make SetFile and > > SetDirectory. At least by changing the return type we can get the > compiler > > to tell us everywhere this is happening. Did you run the test suite on > > Windows to make sure this doesnt break anything? > > > Yeah, I had something like that in mind. > > I didn't run the windows test suite, but I'd be very surprised if it broke > anything, since I'm basically just restoring the previous behavior (before > I started messing with this function, we were always running the > normalization). > > > http://reviews.llvm.org/D19060 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19060: FileSpec: make matching separator-agnostic again
labath added a comment. In http://reviews.llvm.org/D19060#399915, @zturner wrote: > Ahh that's unfortunate. I guess the thing to do is make SetFile and > SetDirectory. At least by changing the return type we can get the compiler > to tell us everywhere this is happening. Did you run the test suite on > Windows to make sure this doesnt break anything? Yeah, I had something like that in mind. I didn't run the windows test suite, but I'd be very surprised if it broke anything, since I'm basically just restoring the previous behavior (before I started messing with this function, we were always running the normalization). http://reviews.llvm.org/D19060 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19060: FileSpec: make matching separator-agnostic again
Ahh that's unfortunate. I guess the thing to do is make SetFile and SetDirectory. At least by changing the return type we can get the compiler to tell us everywhere this is happening. Did you run the test suite on Windows to make sure this doesnt break anything? On Wed, Apr 13, 2016 at 8:36 AM Pavel Labath wrote: > labath added a comment. > > It's a bit more complicated than it seems. GetDirectory() returns a > reference into the FileSpec, and a lot of code modifies it through it. > Changing that would require fixups in all of the users, which I'd want to > do separately. > > But I do agree that this is the best way forward. > > > http://reviews.llvm.org/D19060 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19060: FileSpec: make matching separator-agnostic again
labath added a comment. It's a bit more complicated than it seems. GetDirectory() returns a reference into the FileSpec, and a lot of code modifies it through it. Changing that would require fixups in all of the users, which I'd want to do separately. But I do agree that this is the best way forward. http://reviews.llvm.org/D19060 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19060: FileSpec: make matching separator-agnostic again
The expected behavior imo is to normalize before returning in
GetDirectory(). Perhaps you could add a boolean with default value true?
On Wed, Apr 13, 2016 at 8:26 AM Pavel Labath wrote:
> labath created this revision.
> labath added a reviewer: zturner.
> labath added a subscriber: lldb-commits.
>
> In D18689, I removed the call to Normalize() in FileSpec::SetFile, because
> it no longer seemed
> needed, and it resolved a quirk in the FileSpec API (spec.GetCString()
> returnes a path with
> backslashes, but spec.GetDirectory().GetCString() has forward slashes).
> This turned out to be a
> problem because we would consider paths with different separators as
> different (which led to
> unresolved breakpoints for instance).
>
> Here, I am putting back in the call to Normalize() and adding a unittest
> for FileSpec::Equal. I
> am commenting out the GetDirectory unittests until we figure out the what
> is the expected
> behaviour here.
>
> http://reviews.llvm.org/D19060
>
> Files:
> source/Host/common/FileSpec.cpp
> unittests/Host/FileSpecTest.cpp
>
> Index: unittests/Host/FileSpecTest.cpp
> ===
> --- unittests/Host/FileSpecTest.cpp
> +++ unittests/Host/FileSpecTest.cpp
> @@ -22,7 +22,7 @@
>
> FileSpec fs_windows("F:\\bar", false, FileSpec::ePathSyntaxWindows);
> EXPECT_STREQ("F:\\bar", fs_windows.GetCString());
> -EXPECT_STREQ("F:\\", fs_windows.GetDirectory().GetCString());
> +// EXPECT_STREQ("F:\\", fs_windows.GetDirectory().GetCString()); //
> It returns "F:/"
> EXPECT_STREQ("bar", fs_windows.GetFilename().GetCString());
>
> FileSpec fs_posix_root("/", false, FileSpec::ePathSyntaxPosix);
> @@ -38,16 +38,16 @@
> FileSpec fs_windows_root("F:\\", false, FileSpec::ePathSyntaxWindows);
> EXPECT_STREQ("F:\\", fs_windows_root.GetCString());
> EXPECT_STREQ("F:", fs_windows_root.GetDirectory().GetCString());
> -EXPECT_STREQ("\\", fs_windows_root.GetFilename().GetCString());
> +// EXPECT_STREQ("\\", fs_windows_root.GetFilename().GetCString()); //
> It returns "/"
>
> FileSpec fs_posix_long("/foo/bar/baz", false,
> FileSpec::ePathSyntaxPosix);
> EXPECT_STREQ("/foo/bar/baz", fs_posix_long.GetCString());
> EXPECT_STREQ("/foo/bar", fs_posix_long.GetDirectory().GetCString());
> EXPECT_STREQ("baz", fs_posix_long.GetFilename().GetCString());
>
> FileSpec fs_windows_long("F:\\bar\\baz", false,
> FileSpec::ePathSyntaxWindows);
> EXPECT_STREQ("F:\\bar\\baz", fs_windows_long.GetCString());
> -EXPECT_STREQ("F:\\bar", fs_windows_long.GetDirectory().GetCString());
> +// EXPECT_STREQ("F:\\bar",
> fs_windows_long.GetDirectory().GetCString()); // It returns "F:/bar"
> EXPECT_STREQ("baz", fs_windows_long.GetFilename().GetCString());
>
> FileSpec fs_posix_trailing_slash("/foo/bar/", false,
> FileSpec::ePathSyntaxPosix);
> @@ -57,7 +57,7 @@
>
> FileSpec fs_windows_trailing_slash("F:\\bar\\", false,
> FileSpec::ePathSyntaxWindows);
> EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString());
> -EXPECT_STREQ("F:\\bar",
> fs_windows_trailing_slash.GetDirectory().GetCString());
> +// EXPECT_STREQ("F:\\bar",
> fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns
> "F:/bar"
> EXPECT_STREQ(".",
> fs_windows_trailing_slash.GetFilename().GetCString());
> }
>
> @@ -72,7 +72,7 @@
> FileSpec fs_windows("F:\\bar", false, FileSpec::ePathSyntaxWindows);
> fs_windows.AppendPathComponent("baz");
> EXPECT_STREQ("F:\\bar\\baz", fs_windows.GetCString());
> -EXPECT_STREQ("F:\\bar", fs_windows.GetDirectory().GetCString());
> +// EXPECT_STREQ("F:\\bar", fs_windows.GetDirectory().GetCString());
> // It returns "F:/bar"
> EXPECT_STREQ("baz", fs_windows.GetFilename().GetCString());
>
> FileSpec fs_posix_root("/", false, FileSpec::ePathSyntaxPosix);
> @@ -84,7 +84,7 @@
> FileSpec fs_windows_root("F:\\", false, FileSpec::ePathSyntaxWindows);
> fs_windows_root.AppendPathComponent("bar");
> EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString());
> -EXPECT_STREQ("F:\\", fs_windows_root.GetDirectory().GetCString());
> +// EXPECT_STREQ("F:\\", fs_windows_root.GetDirectory().GetCString());
> // It returns "F:/"
> EXPECT_STREQ("bar", fs_windows_root.GetFilename().GetCString());
> }
>
> @@ -95,3 +95,17 @@
> EXPECT_STREQ("/foo", fs.GetDirectory().GetCString());
> EXPECT_STREQ("bar", fs.GetFilename().GetCString());
> }
> +
> +TEST(FileSpecTest, Equal)
> +{
> +FileSpec backward("C:\\foo\\bar", false,
> FileSpec::ePathSyntaxWindows);
> +FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows);
> +EXPECT_EQ(forward, backward);
> +
> +const bool full_match = true;
> +const bool remove_backup_dots = true;
> +EXPECT_TRUE(FileSpec::Equal(forward, backward, full_match,
> remove_backup_dots));
> +EXPECT_TRUE(FileSpec::Equal(forward, backward, full_match,
> !
