[Lldb-commits] [PATCH] D151399: [lldb] Introduce FileSpec::GetComponents
This revision was automatically updated to reflect the committed changes.
Closed by commit rG57154a63a07f: [lldb] Introduce FileSpec::GetComponents
(authored by bulbazord).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151399/new/
https://reviews.llvm.org/D151399
Files:
lldb/include/lldb/Utility/FileSpec.h
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
lldb/source/Utility/FileSpec.cpp
lldb/unittests/Utility/FileSpecTest.cpp
Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -504,3 +504,33 @@
EXPECT_FALSE(win_noext.IsSourceImplementationFile());
EXPECT_FALSE(exe.IsSourceImplementationFile());
}
+
+TEST(FileSpecTest, TestGetComponents) {
+ std::pair> PosixTests[] = {
+ {"/", {}},
+ {"/foo", {"foo"}},
+ {"/foo/", {"foo"}},
+ {"/foo/bar", {"foo", "bar"}},
+ {"/llvm-project/lldb/unittests/Utility/FileSpecTest.cpp",
+ {"llvm-project", "lldb", "unittests", "Utility", "FileSpecTest.cpp"}},
+ };
+
+ for (const auto &pair : PosixTests) {
+FileSpec file_spec = PosixSpec(pair.first);
+EXPECT_EQ(file_spec.GetComponents(), pair.second);
+ }
+
+ std::pair> WindowsTests[] = {
+ {"C:\\", {"C:"}},
+ {"C:\\Windows\\", {"C:", "Windows"}},
+ {"C:\\Windows\\System32", {"C:", "Windows", "System32"}},
+ {"C:\\llvm-project\\lldb\\unittests\\Utility\\FileSpecTest.cpp",
+ {"C:", "llvm-project", "lldb", "unittests", "Utility",
+"FileSpecTest.cpp"}},
+ };
+
+ for (const auto &pair : WindowsTests) {
+FileSpec file_spec = WindowsSpec(pair.first);
+EXPECT_EQ(file_spec.GetComponents(), pair.second);
+ }
+}
Index: lldb/source/Utility/FileSpec.cpp
===
--- lldb/source/Utility/FileSpec.cpp
+++ lldb/source/Utility/FileSpec.cpp
@@ -463,6 +463,26 @@
}
return false;
}
+
+std::vector FileSpec::GetComponents() const {
+ std::vector components;
+
+ auto dir_begin = llvm::sys::path::begin(m_directory.GetStringRef(), m_style);
+ auto dir_end = llvm::sys::path::end(m_directory.GetStringRef());
+
+ for (auto iter = dir_begin; iter != dir_end; ++iter) {
+if (*iter == "/" || *iter == ".")
+ continue;
+
+components.push_back(*iter);
+ }
+
+ if (!m_filename.IsEmpty() && m_filename != "/" && m_filename != ".")
+components.push_back(m_filename.GetStringRef());
+
+ return components;
+}
+
/// Returns true if the filespec represents an implementation source
/// file (files with a ".c", ".cpp", ".m", ".mm" (many more)
/// extension).
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -1236,13 +1236,9 @@
// "UIFoundation" and "UIFoundation.framework" -- most likely the latter
// will be the one we find there.
-FileSpec platform_pull_upart(platform_file);
-std::vector path_parts;
-path_parts.push_back(platform_pull_upart.GetFilename().AsCString());
-while (platform_pull_upart.RemoveLastPathComponent()) {
- ConstString part = platform_pull_upart.GetFilename();
- path_parts.push_back(part.AsCString());
-}
+std::vector path_parts = platform_file.GetComponents();
+// We want the components in reverse order.
+std::reverse(path_parts.begin(), path_parts.end());
const size_t path_parts_size = path_parts.size();
size_t num_module_search_paths = module_search_paths_ptr->GetSize();
Index: lldb/include/lldb/Utility/FileSpec.h
===
--- lldb/include/lldb/Utility/FileSpec.h
+++ lldb/include/lldb/Utility/FileSpec.h
@@ -408,6 +408,18 @@
/// A boolean value indicating whether the path was updated.
bool RemoveLastPathComponent();
+ /// Gets the components of the FileSpec's path.
+ /// For example, given the path:
+ /// /System/Library/PrivateFrameworks/UIFoundation.framework/UIFoundation
+ ///
+ /// This function returns:
+ /// {"System", "Library", "PrivateFrameworks", "UIFoundation.framework",
+ /// "UIFoundation"}
+ /// \return
+ /// A std::vector of llvm::StringRefs for each path component.
+ /// The lifetime of the StringRefs is tied to the lifetime of the FileSpec.
+ std::vector GetComponents() const;
+
protected:
// Convenience method for setting the file without changing the style.
void SetFile(llvm::StringRef path);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D151399: [lldb] Introduce FileSpec::GetComponents
mib accepted this revision. mib added a comment. This revision is now accepted and ready to land. LGTM! Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:1241 +// We want the components in reverse order. +std::reverse(path_parts.begin(), path_parts.end()); const size_t path_parts_size = path_parts.size(); Cool! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151399/new/ https://reviews.llvm.org/D151399 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D151399: [lldb] Introduce FileSpec::GetComponents
bulbazord updated this revision to Diff 525757.
bulbazord added a comment.
Convert return type to `std::vector`
Actually fix code so this thing compiles correctly
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151399/new/
https://reviews.llvm.org/D151399
Files:
lldb/include/lldb/Utility/FileSpec.h
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
lldb/source/Utility/FileSpec.cpp
lldb/unittests/Utility/FileSpecTest.cpp
Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -504,3 +504,33 @@
EXPECT_FALSE(win_noext.IsSourceImplementationFile());
EXPECT_FALSE(exe.IsSourceImplementationFile());
}
+
+TEST(FileSpecTest, TestGetComponents) {
+ std::pair> PosixTests[] = {
+ {"/", {}},
+ {"/foo", {"foo"}},
+ {"/foo/", {"foo"}},
+ {"/foo/bar", {"foo", "bar"}},
+ {"/llvm-project/lldb/unittests/Utility/FileSpecTest.cpp",
+ {"llvm-project", "lldb", "unittests", "Utility", "FileSpecTest.cpp"}},
+ };
+
+ for (const auto &pair : PosixTests) {
+FileSpec file_spec = PosixSpec(pair.first);
+EXPECT_EQ(file_spec.GetComponents(), pair.second);
+ }
+
+ std::pair> WindowsTests[] = {
+ {"C:\\", {"C:"}},
+ {"C:\\Windows\\", {"C:", "Windows"}},
+ {"C:\\Windows\\System32", {"C:", "Windows", "System32"}},
+ {"C:\\llvm-project\\lldb\\unittests\\Utility\\FileSpecTest.cpp",
+ {"C:", "llvm-project", "lldb", "unittests", "Utility",
+"FileSpecTest.cpp"}},
+ };
+
+ for (const auto &pair : WindowsTests) {
+FileSpec file_spec = WindowsSpec(pair.first);
+EXPECT_EQ(file_spec.GetComponents(), pair.second);
+ }
+}
Index: lldb/source/Utility/FileSpec.cpp
===
--- lldb/source/Utility/FileSpec.cpp
+++ lldb/source/Utility/FileSpec.cpp
@@ -463,6 +463,26 @@
}
return false;
}
+
+std::vector FileSpec::GetComponents() const {
+ std::vector components;
+
+ auto dir_begin = llvm::sys::path::begin(m_directory.GetStringRef(), m_style);
+ auto dir_end = llvm::sys::path::end(m_directory.GetStringRef());
+
+ for (auto iter = dir_begin; iter != dir_end; ++iter) {
+if (*iter == "/" || *iter == ".")
+ continue;
+
+components.push_back(*iter);
+ }
+
+ if (!m_filename.IsEmpty() && m_filename != "/" && m_filename != ".")
+components.push_back(m_filename.GetStringRef());
+
+ return components;
+}
+
/// Returns true if the filespec represents an implementation source
/// file (files with a ".c", ".cpp", ".m", ".mm" (many more)
/// extension).
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -1236,13 +1236,9 @@
// "UIFoundation" and "UIFoundation.framework" -- most likely the latter
// will be the one we find there.
-FileSpec platform_pull_upart(platform_file);
-std::vector path_parts;
-path_parts.push_back(platform_pull_upart.GetFilename().AsCString());
-while (platform_pull_upart.RemoveLastPathComponent()) {
- ConstString part = platform_pull_upart.GetFilename();
- path_parts.push_back(part.AsCString());
-}
+std::vector path_parts = platform_file.GetComponents();
+// We want the components in reverse order.
+std::reverse(path_parts.begin(), path_parts.end());
const size_t path_parts_size = path_parts.size();
size_t num_module_search_paths = module_search_paths_ptr->GetSize();
Index: lldb/include/lldb/Utility/FileSpec.h
===
--- lldb/include/lldb/Utility/FileSpec.h
+++ lldb/include/lldb/Utility/FileSpec.h
@@ -408,6 +408,18 @@
/// A boolean value indicating whether the path was updated.
bool RemoveLastPathComponent();
+ /// Gets the components of the FileSpec's path.
+ /// For example, given the path:
+ /// /System/Library/PrivateFrameworks/UIFoundation.framework/UIFoundation
+ ///
+ /// This function returns:
+ /// {"System", "Library", "PrivateFrameworks", "UIFoundation.framework",
+ /// "UIFoundation"}
+ /// \return
+ /// A std::vector of llvm::StringRefs for each path component.
+ /// The lifetime of the StringRefs is tied to the lifetime of the FileSpec.
+ std::vector GetComponents() const;
+
protected:
// Convenience method for setting the file without changing the style.
void SetFile(llvm::StringRef path);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D151399: [lldb] Introduce FileSpec::GetComponents
bulbazord added inline comments. Comment at: lldb/include/lldb/Utility/FileSpec.h:420 + /// A std::vector of std::strings for each path component. + std::vector GetComponents() const; + JDevlieghere wrote: > bulbazord wrote: > > JDevlieghere wrote: > > > I'm surprised this returns a vector of `std::string`s and not > > > `llvm::StringRef`s. I would expect all the components to be part of the > > > FileSpec's stored path. Even with the file and directory stored as > > > separate `ConstString`s, that should be possible? > > Yes, it is possible to do `std::vector` here, especially > > because they would be backed by `ConstString`s which live forever. I chose > > `std::string` here because of the possibility that we one day no longer use > > `ConstString` to store the path, in which case the vector's StringRefs > > would only be valid for the lifetime of the FileSpec (or until it gets > > mutated). > > > > Maybe I'm thinking too far ahead or planning for a future that will never > > come though. What do you think? > I think it's reasonable to expect the lifetime of things handed out by a > FileSpec match the lifetime of the FileSpec, but that depends on how we want > to deal with mutability. If we want to be able to mutate a FileSpec in place, > then you're right, these things need to have their own lifetime. I'd prefer to move towards FileSpec being immutable (or as close as possible), so I'll update this and change it to `StringRef. Thanks for the feedback! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151399/new/ https://reviews.llvm.org/D151399 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D151399: [lldb] Introduce FileSpec::GetComponents
JDevlieghere added inline comments. Comment at: lldb/include/lldb/Utility/FileSpec.h:420 + /// A std::vector of std::strings for each path component. + std::vector GetComponents() const; + bulbazord wrote: > JDevlieghere wrote: > > I'm surprised this returns a vector of `std::string`s and not > > `llvm::StringRef`s. I would expect all the components to be part of the > > FileSpec's stored path. Even with the file and directory stored as separate > > `ConstString`s, that should be possible? > Yes, it is possible to do `std::vector` here, especially > because they would be backed by `ConstString`s which live forever. I chose > `std::string` here because of the possibility that we one day no longer use > `ConstString` to store the path, in which case the vector's StringRefs would > only be valid for the lifetime of the FileSpec (or until it gets mutated). > > Maybe I'm thinking too far ahead or planning for a future that will never > come though. What do you think? I think it's reasonable to expect the lifetime of things handed out by a FileSpec match the lifetime of the FileSpec, but that depends on how we want to deal with mutability. If we want to be able to mutate a FileSpec in place, then you're right, these things need to have their own lifetime. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151399/new/ https://reviews.llvm.org/D151399 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D151399: [lldb] Introduce FileSpec::GetComponents
bulbazord added inline comments. Comment at: lldb/include/lldb/Utility/FileSpec.h:420 + /// A std::vector of std::strings for each path component. + std::vector GetComponents() const; + JDevlieghere wrote: > I'm surprised this returns a vector of `std::string`s and not > `llvm::StringRef`s. I would expect all the components to be part of the > FileSpec's stored path. Even with the file and directory stored as separate > `ConstString`s, that should be possible? Yes, it is possible to do `std::vector` here, especially because they would be backed by `ConstString`s which live forever. I chose `std::string` here because of the possibility that we one day no longer use `ConstString` to store the path, in which case the vector's StringRefs would only be valid for the lifetime of the FileSpec (or until it gets mutated). Maybe I'm thinking too far ahead or planning for a future that will never come though. What do you think? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151399/new/ https://reviews.llvm.org/D151399 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D151399: [lldb] Introduce FileSpec::GetComponents
JDevlieghere added inline comments. Comment at: lldb/include/lldb/Utility/FileSpec.h:420 + /// A std::vector of std::strings for each path component. + std::vector GetComponents() const; + I'm surprised this returns a vector of `std::string`s and not `llvm::StringRef`s. I would expect all the components to be part of the FileSpec's stored path. Even with the file and directory stored as separate `ConstString`s, that should be possible? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151399/new/ https://reviews.llvm.org/D151399 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D151399: [lldb] Introduce FileSpec::GetComponents
bulbazord created this revision.
bulbazord added reviewers: JDevlieghere, jingham, mib, jasonmolenda.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
This patch introduces FileSpec::GetComponents, a method that splits a
FileSpec's path into its individual components. For example, given
/foo/bar/baz, you'll get back a vector of strings {"foo", "bar", baz"}.
The motivation here is to reduce the use of
`FileSpec::RemoveLastPathComponent`. Mutating a FileSpec is expensive,
so providing a way of doing this without mutation is useful.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D151399
Files:
lldb/include/lldb/Utility/FileSpec.h
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
lldb/source/Utility/FileSpec.cpp
lldb/unittests/Utility/FileSpecTest.cpp
Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -504,3 +504,33 @@
EXPECT_FALSE(win_noext.IsSourceImplementationFile());
EXPECT_FALSE(exe.IsSourceImplementationFile());
}
+
+TEST(FileSpecTest, TestGetComponents) {
+ std::pair> PosixTests[] = {
+ {"/", {}},
+ {"/foo", {"foo"}},
+ {"/foo/", {"foo"}},
+ {"/foo/bar", {"foo", "bar"}},
+ {"/llvm-project/lldb/unittests/Utility/FileSpecTest.cpp",
+ {"llvm-project", "lldb", "unittests", "Utility", "FileSpecTest.cpp"}},
+ };
+
+ for (const auto &pair : PosixTests) {
+FileSpec file_spec = PosixSpec(pair.first);
+EXPECT_EQ(file_spec.GetComponents(), pair.second);
+ }
+
+ std::pair> WindowsTests[] = {
+ {"C:\\", {"C:"}},
+ {"C:\\Windows\\", {"C:", "Windows"}},
+ {"C:\\Windows\\System32", {"C:", "Windows", "System32"}},
+ {"C:\\llvm-project\\lldb\\unittests\\Utility\\FileSpecTest.cpp",
+ {"C:", "llvm-project", "lldb", "unittests", "Utility",
+"FileSpecTest.cpp"}},
+ };
+
+ for (const auto &pair : WindowsTests) {
+FileSpec file_spec = WindowsSpec(pair.first);
+EXPECT_EQ(file_spec.GetComponents(), pair.second);
+ }
+}
Index: lldb/source/Utility/FileSpec.cpp
===
--- lldb/source/Utility/FileSpec.cpp
+++ lldb/source/Utility/FileSpec.cpp
@@ -463,6 +463,26 @@
}
return false;
}
+
+std::vector FileSpec::GetComponents() const {
+ std::vector components;
+
+ llvm::SmallString<64> current_path;
+ GetPath(current_path, false);
+
+ auto begin = llvm::sys::path::begin(current_path, m_style);
+ auto end = llvm::sys::path::end(current_path);
+
+ for (auto iter = begin; iter != end; ++iter) {
+if (*iter == "/" || *iter == ".")
+ continue;
+
+components.push_back(iter->str());
+ }
+
+ return components;
+}
+
/// Returns true if the filespec represents an implementation source
/// file (files with a ".c", ".cpp", ".m", ".mm" (many more)
/// extension).
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -1236,13 +1236,9 @@
// "UIFoundation" and "UIFoundation.framework" -- most likely the latter
// will be the one we find there.
-FileSpec platform_pull_upart(platform_file);
-std::vector path_parts;
-path_parts.push_back(platform_pull_upart.GetFilename().AsCString());
-while (platform_pull_upart.RemoveLastPathComponent()) {
- ConstString part = platform_pull_upart.GetFilename();
- path_parts.push_back(part.AsCString());
-}
+std::vector path_components = platform_file.GetComponents();
+// We want the components in reverse order.
+std::reverse(path_components.begin(), path_components.end());
const size_t path_parts_size = path_parts.size();
size_t num_module_search_paths = module_search_paths_ptr->GetSize();
Index: lldb/include/lldb/Utility/FileSpec.h
===
--- lldb/include/lldb/Utility/FileSpec.h
+++ lldb/include/lldb/Utility/FileSpec.h
@@ -408,6 +408,17 @@
/// A boolean value indicating whether the path was updated.
bool RemoveLastPathComponent();
+ /// Gets the components of the FileSpec's path.
+ /// For example, given the path:
+ /// /System/Library/PrivateFrameworks/UIFoundation.framework/UIFoundation
+ ///
+ /// This function returns:
+ /// {"System", "Library", "PrivateFrameworks", "UIFoundation.framework",
+ /// "UIFoundation"}
+ /// \return
+ /// A std::vector of std::strings for each path component.
+ std::vector GetComponents() const;
+
protected:
// Convenience method for setting the file without changing the style.
void SetFile(llvm::StringRef path);
___
lldb-commit
