Re: [Lldb-commits] [PATCH] D18044: Fix a couple of cornercases in FileSpec + tests
Yea, because of the fact that we must support both syntaxes on both
platforms, LLVM library is out.
The whole motivation for introducing the path syntax is so that windows
paths behave as if on Windows even if on linux
On Wed, Mar 16, 2016 at 6:19 AM Pavel Labath wrote:
> labath added inline comments.
>
>
> Comment at: lldb/trunk/unittests/Host/FileSpecTest.cpp:25
> @@ +24,3 @@
> +EXPECT_STREQ("F:\\bar", fs_windows.GetCString());
> +EXPECT_STREQ("F:", fs_windows.GetDirectory().GetCString());
> +EXPECT_STREQ("bar", fs_windows.GetFilename().GetCString());
>
> So this returns `F:` on linux and `F:\` on Windows. This happens because
> `llvm::sys::path::parent_path` does not recognize `F:` as a "root
> directory" on linux, and therefore treats it differently. I don't know
> which behavior is more "correct" (probably the windows one), but I think
> that this should be consistent, regardless of the platform the test is run
> on (my original motivation for writing this was the fact that i was getting
> wonky paths while attempting to write other unit tests). Unfortunately, I
> think that means getting rid of llvm's path processing library...
>
> What do you make of that?
>
> (I am going on holiday, so I cannot to anything about this now. if you
> want to have a clean test run in the mean time, I am fine commenting these
> checks out or something...)
>
>
> Repository:
> rL LLVM
>
> http://reviews.llvm.org/D18044
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D18044: Fix a couple of cornercases in FileSpec + tests
labath added inline comments.
Comment at: lldb/trunk/unittests/Host/FileSpecTest.cpp:25
@@ +24,3 @@
+EXPECT_STREQ("F:\\bar", fs_windows.GetCString());
+EXPECT_STREQ("F:", fs_windows.GetDirectory().GetCString());
+EXPECT_STREQ("bar", fs_windows.GetFilename().GetCString());
So this returns `F:` on linux and `F:\` on Windows. This happens because
`llvm::sys::path::parent_path` does not recognize `F:` as a "root directory" on
linux, and therefore treats it differently. I don't know which behavior is more
"correct" (probably the windows one), but I think that this should be
consistent, regardless of the platform the test is run on (my original
motivation for writing this was the fact that i was getting wonky paths while
attempting to write other unit tests). Unfortunately, I think that means
getting rid of llvm's path processing library...
What do you make of that?
(I am going on holiday, so I cannot to anything about this now. if you want to
have a clean test run in the mean time, I am fine commenting these checks out
or something...)
Repository:
rL LLVM
http://reviews.llvm.org/D18044
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D18044: Fix a couple of cornercases in FileSpec + tests
labath added a comment. Interesting... I didn't run the tests on windows. I was hoping they would run mostly the same on all platforms. I'll try to give them a spin now... Repository: rL LLVM http://reviews.llvm.org/D18044 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D18044: Fix a couple of cornercases in FileSpec + tests
This actually broke a couple of unit tests on Windows. Did you try running these tests on Windows? On Fri, Mar 11, 2016 at 12:49 AM Pavel Labath wrote: > This revision was automatically updated to reflect the committed changes. > Closed by commit rL263207: Fix a couple of cornercases in FileSpec + tests > (authored by labath). > > Changed prior to commit: > http://reviews.llvm.org/D18044?vs=50275&id=50396#toc > > Repository: > rL LLVM > > http://reviews.llvm.org/D18044 > > Files: > lldb/trunk/source/Host/common/FileSpec.cpp > lldb/trunk/unittests/Host/CMakeLists.txt > lldb/trunk/unittests/Host/FileSpecTest.cpp > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D18044: Fix a couple of cornercases in FileSpec + tests
This revision was automatically updated to reflect the committed changes.
Closed by commit rL263207: Fix a couple of cornercases in FileSpec + tests
(authored by labath).
Changed prior to commit:
http://reviews.llvm.org/D18044?vs=50275&id=50396#toc
Repository:
rL LLVM
http://reviews.llvm.org/D18044
Files:
lldb/trunk/source/Host/common/FileSpec.cpp
lldb/trunk/unittests/Host/CMakeLists.txt
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
@@ -0,0 +1,94 @@
+//===-- FileSpecTest.cpp *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Host/FileSpec.h"
+
+using namespace lldb_private;
+
+TEST(FileSpecTest, FileAndDirectoryComponents)
+{
+FileSpec fs_posix("/foo/bar", false, FileSpec::ePathSyntaxPosix);
+EXPECT_STREQ("/foo/bar", fs_posix.GetCString());
+EXPECT_STREQ("/foo", fs_posix.GetDirectory().GetCString());
+EXPECT_STREQ("bar", fs_posix.GetFilename().GetCString());
+
+FileSpec fs_windows("F:\\bar", false, FileSpec::ePathSyntaxWindows);
+EXPECT_STREQ("F:\\bar", fs_windows.GetCString());
+EXPECT_STREQ("F:", fs_windows.GetDirectory().GetCString());
+EXPECT_STREQ("bar", fs_windows.GetFilename().GetCString());
+
+FileSpec fs_posix_root("/", false, FileSpec::ePathSyntaxPosix);
+EXPECT_STREQ("/", fs_posix_root.GetCString());
+EXPECT_EQ(nullptr, fs_posix_root.GetDirectory().GetCString());
+EXPECT_STREQ("/", fs_posix_root.GetFilename().GetCString());
+
+FileSpec fs_windows_root("F:", false, FileSpec::ePathSyntaxWindows);
+EXPECT_STREQ("F:", fs_windows_root.GetCString());
+EXPECT_EQ(nullptr, fs_windows_root.GetDirectory().GetCString());
+EXPECT_STREQ("F:", fs_windows_root.GetFilename().GetCString());
+
+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());
+// We get "F:/bar" instead.
+// EXPECT_STREQ("F:\\bar", fs_windows_long.GetDirectory().GetCString());
+EXPECT_STREQ("baz", fs_windows_long.GetFilename().GetCString());
+
+FileSpec fs_posix_trailing_slash("/foo/bar/", false, FileSpec::ePathSyntaxPosix);
+EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString());
+EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString());
+EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString());
+
+FileSpec fs_windows_trailing_slash("F:\\bar\\", false, FileSpec::ePathSyntaxWindows);
+EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString());
+// We get "F:/bar" instead.
+// EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetDirectory().GetCString());
+EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString());
+}
+
+TEST(FileSpecTest, AppendPathComponent)
+{
+FileSpec fs_posix("/foo", false, FileSpec::ePathSyntaxPosix);
+fs_posix.AppendPathComponent("bar");
+EXPECT_STREQ("/foo/bar", fs_posix.GetCString());
+EXPECT_STREQ("/foo", fs_posix.GetDirectory().GetCString());
+EXPECT_STREQ("bar", fs_posix.GetFilename().GetCString());
+
+FileSpec fs_windows("F:", false, FileSpec::ePathSyntaxWindows);
+fs_windows.AppendPathComponent("bar");
+EXPECT_STREQ("F:\\bar", fs_windows.GetCString());
+EXPECT_STREQ("F:", fs_windows.GetDirectory().GetCString());
+EXPECT_STREQ("bar", fs_windows.GetFilename().GetCString());
+
+FileSpec fs_posix_root("/", false, FileSpec::ePathSyntaxPosix);
+fs_posix_root.AppendPathComponent("bar");
+EXPECT_STREQ("/bar", fs_posix_root.GetCString());
+EXPECT_STREQ("/", fs_posix_root.GetDirectory().GetCString());
+EXPECT_STREQ("bar", fs_posix_root.GetFilename().GetCString());
+
+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("bar", fs_windows_root.GetFilename().GetCString());
+}
+
+TEST(FileSpecTest, CopyByAppendingPathComponent)
+{
+FileSpec fs = FileSpec("/foo", false, FileSpec::ePathSyntaxPosix).CopyByAppendingPathComponent("bar");
+EXPECT_STREQ("/foo/bar", fs.GetCS
Re: [Lldb-commits] [PATCH] D18044: Fix a couple of cornercases in FileSpec + tests
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good. http://reviews.llvm.org/D18044 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D18044: Fix a couple of cornercases in FileSpec + tests
labath added a comment.
Comment at: unittests/Host/FileSpecTest.cpp:57
@@ +56,3 @@
+// We get "F:/bar" instead.
+// EXPECT_STREQ("F:\\bar",
fs_windows_trailing_slash.GetDirectory().GetCString());
+EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString());
I've also identified one more issue in windows path handling (GetDirectory()
always returns posix-style paths), which I do not attempt to address right now.
http://reviews.llvm.org/D18044
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
