[Lldb-commits] [PATCH] D47495: [FileSpec] Re-implmenet removeLastPathComponent

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL333540: [FileSpec] Re-implmenet removeLastPathComponent 
(authored by JDevlieghere, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D47495?vs=149083=149102#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D47495

Files:
  lldb/trunk/include/lldb/Utility/FileSpec.h
  lldb/trunk/source/Utility/FileSpec.cpp
  lldb/trunk/unittests/Utility/FileSpecTest.cpp

Index: lldb/trunk/unittests/Utility/FileSpecTest.cpp
===
--- lldb/trunk/unittests/Utility/FileSpecTest.cpp
+++ lldb/trunk/unittests/Utility/FileSpecTest.cpp
@@ -320,3 +320,44 @@
   }
 }
 
+TEST(FileSpecTest, RemoveLastPathComponent) {
+  FileSpec fs_posix("/foo/bar/baz", false, FileSpec::Style::posix);
+  EXPECT_STREQ("/foo/bar/baz", fs_posix.GetCString());
+  EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+  EXPECT_STREQ("/foo/bar", fs_posix.GetCString());
+  EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+  EXPECT_STREQ("/foo", fs_posix.GetCString());
+  EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+  EXPECT_STREQ("/", fs_posix.GetCString());
+  EXPECT_FALSE(fs_posix.RemoveLastPathComponent());
+  EXPECT_STREQ("/", fs_posix.GetCString());
+
+  FileSpec fs_posix_relative("./foo/bar/baz", false, FileSpec::Style::posix);
+  EXPECT_STREQ("foo/bar/baz", fs_posix_relative.GetCString());
+  EXPECT_TRUE(fs_posix_relative.RemoveLastPathComponent());
+  EXPECT_STREQ("foo/bar", fs_posix_relative.GetCString());
+  EXPECT_TRUE(fs_posix_relative.RemoveLastPathComponent());
+  EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+  EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+  EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+
+  FileSpec fs_posix_relative2("./", false, FileSpec::Style::posix);
+  EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+  EXPECT_FALSE(fs_posix_relative2.RemoveLastPathComponent());
+  EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+  EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+  EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+
+  FileSpec fs_windows("C:\\foo\\bar\\baz", false, FileSpec::Style::windows);
+  EXPECT_STREQ("C:\\foo\\bar\\baz", fs_windows.GetCString());
+  EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+  EXPECT_STREQ("C:\\foo\\bar", fs_windows.GetCString());
+  EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+  EXPECT_STREQ("C:\\foo", fs_windows.GetCString());
+  EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+  EXPECT_STREQ("C:\\", fs_windows.GetCString());
+  EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+  EXPECT_STREQ("C:", fs_windows.GetCString());
+  EXPECT_FALSE(fs_windows.RemoveLastPathComponent());
+  EXPECT_STREQ("C:", fs_windows.GetCString());
+}
Index: lldb/trunk/source/Utility/FileSpec.cpp
===
--- lldb/trunk/source/Utility/FileSpec.cpp
+++ lldb/trunk/source/Utility/FileSpec.cpp
@@ -785,36 +785,15 @@
   return AppendPathComponent(new_path.GetPath(false));
 }
 
-void FileSpec::RemoveLastPathComponent() {
-  // CLEANUP: Use StringRef for string handling.
-
-  const bool resolve = false;
-  if (m_filename.IsEmpty() && m_directory.IsEmpty()) {
-SetFile("", resolve);
-return;
-  }
-  if (m_directory.IsEmpty()) {
-SetFile("", resolve);
-return;
+bool FileSpec::RemoveLastPathComponent() {
+  llvm::SmallString<64> current_path;
+  GetPath(current_path, false);
+  if (llvm::sys::path::has_parent_path(current_path, m_style)) {
+SetFile(llvm::sys::path::parent_path(current_path, m_style), false,
+m_style);
+return true;
   }
-  if (m_filename.IsEmpty()) {
-const char *dir_cstr = m_directory.GetCString();
-const char *last_slash_ptr = ::strrchr(dir_cstr, '/');
-
-// check for obvious cases before doing the full thing
-if (!last_slash_ptr) {
-  SetFile("", resolve);
-  return;
-}
-if (last_slash_ptr == dir_cstr) {
-  SetFile("/", resolve);
-  return;
-}
-size_t last_slash_pos = last_slash_ptr - dir_cstr + 1;
-ConstString new_path(dir_cstr, last_slash_pos);
-SetFile(new_path.GetCString(), resolve);
-  } else
-SetFile(m_directory.GetCString(), resolve);
+  return false;
 }
 //--
 /// Returns true if the filespec represents an implementation source
Index: lldb/trunk/include/lldb/Utility/FileSpec.h
===
--- lldb/trunk/include/lldb/Utility/FileSpec.h
+++ lldb/trunk/include/lldb/Utility/FileSpec.h
@@ -532,7 +532,14 @@
   void AppendPathComponent(llvm::StringRef component);
   void AppendPathComponent(const FileSpec _path);
 
-  void RemoveLastPathComponent();
+  //--
+  /// Removes the last path 

[Lldb-commits] [PATCH] D47495: [FileSpec] Re-implmenet removeLastPathComponent

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.



Comment at: unittests/Utility/FileSpecTest.cpp:342
+  EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+  EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+

JDevlieghere wrote:
> labath wrote:
> > Is this the behavior you want here? I was thinking we could fold this all 
> > the way to "." (arguably "." is a parent of "foo", though I can see how 
> > that may be thought to be too magical)
> I like this approach it doesn't consider `.` to be a special case and 
> therefore things are consistent and straightforward. My worry is that if we 
> add special cases, we risk ending up with missing edge cases (like the 
> previous implementation). 
Ok, if that works for your use case (as far as path remappings go, "." is a 
more generic mapping than "foo"), then that's fine by me. I like how you were 
able to concisely describe the new semantics of this function.


https://reviews.llvm.org/D47495



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D47495: [FileSpec] Re-implmenet removeLastPathComponent

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: unittests/Utility/FileSpecTest.cpp:342
+  EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+  EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+

labath wrote:
> Is this the behavior you want here? I was thinking we could fold this all the 
> way to "." (arguably "." is a parent of "foo", though I can see how that may 
> be thought to be too magical)
I like this approach it doesn't consider `.` to be a special case and therefore 
things are consistent and straightforward. My worry is that if we add special 
cases, we risk ending up with missing edge cases (like the previous 
implementation). 


https://reviews.llvm.org/D47495



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D47495: [FileSpec] Re-implmenet removeLastPathComponent

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: unittests/Utility/FileSpecTest.cpp:342
+  EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+  EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+

Is this the behavior you want here? I was thinking we could fold this all the 
way to "." (arguably "." is a parent of "foo", though I can see how that may be 
thought to be too magical)


https://reviews.llvm.org/D47495



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits