[Lldb-commits] [lldb] r346466 - Revert "[FileSystem] Make use of FS in TildeExpressionResolver"
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Sat Nov 10 02:38:20 PST 2018
I think the right way to resolve this would be to move
"StandardTildeExpressionResolver" into the host module. The non-standard
TildeExpressionResolver is just an abstract class, so it does not need
the filesystem. This way, anyone wanting to use the resolver can just
depend on the interface, and the concrete class may not even have to be
a public one as it the FileSystem can just vend the interface.
Although, at this point it's a question whether we need the resolver
class in the first place. It was introduced so we could mock/test
resolution of paths with usernames in them. However that is something
you will also need to do for the repro feature, so it may make sense to
fold everything into the Filesystem class.
On 09/11/18 02:59, Jonas Devlieghere via lldb-commits wrote:
> Author: jdevlieghere
> Date: Thu Nov 8 17:59:28 2018
> New Revision: 346466
>
> URL: http://llvm.org/viewvc/llvm-project?rev=346466&view=rev
> Log:
> Revert "[FileSystem] Make use of FS in TildeExpressionResolver"
>
> The whole point of this change was making it possible to resolve paths
> without depending on the FileSystem, which is not what I did here. Not
> sure what I was thinking...
>
> Modified:
> lldb/trunk/include/lldb/Host/FileSystem.h
> lldb/trunk/include/lldb/Utility/TildeExpressionResolver.h
> lldb/trunk/source/Commands/CommandCompletions.cpp
> lldb/trunk/source/Host/common/FileSystem.cpp
> lldb/trunk/source/Target/TargetList.cpp
> lldb/trunk/source/Utility/TildeExpressionResolver.cpp
> lldb/trunk/unittests/Host/FileSystemTest.cpp
> lldb/trunk/unittests/Interpreter/TestCompletion.cpp
> lldb/trunk/unittests/TestingSupport/MockTildeExpressionResolver.cpp
> lldb/trunk/unittests/TestingSupport/MockTildeExpressionResolver.h
> lldb/trunk/unittests/Utility/TildeExpressionResolverTest.cpp
>
> Modified: lldb/trunk/include/lldb/Host/FileSystem.h
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/FileSystem.h?rev=346466&r1=346465&r2=346466&view=diff
> ==============================================================================
> --- lldb/trunk/include/lldb/Host/FileSystem.h (original)
> +++ lldb/trunk/include/lldb/Host/FileSystem.h Thu Nov 8 17:59:28 2018
> @@ -132,8 +132,7 @@ public:
> void *callback_baton);
>
> std::error_code GetRealPath(const llvm::Twine &path,
> - llvm::SmallVectorImpl<char> &output,
> - bool expand_tilde) const;
> + llvm::SmallVectorImpl<char> &output) const;
>
> private:
> static llvm::Optional<FileSystem> &InstanceImpl();
>
> Modified: lldb/trunk/include/lldb/Utility/TildeExpressionResolver.h
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/TildeExpressionResolver.h?rev=346466&r1=346465&r2=346466&view=diff
> ==============================================================================
> --- lldb/trunk/include/lldb/Utility/TildeExpressionResolver.h (original)
> +++ lldb/trunk/include/lldb/Utility/TildeExpressionResolver.h Thu Nov 8 17:59:28 2018
> @@ -18,10 +18,8 @@ template <typename T> class SmallVectorI
> }
>
> namespace lldb_private {
> -class FileSystem;
> class TildeExpressionResolver {
> public:
> - TildeExpressionResolver(FileSystem &fs) : m_fs(fs) {}
> virtual ~TildeExpressionResolver();
>
> /// Resolve a Tilde Expression contained according to bash rules.
> @@ -54,20 +52,14 @@ public:
> /// the username portion with the matched result.
> bool ResolveFullPath(llvm::StringRef Expr,
> llvm::SmallVectorImpl<char> &Output);
> -
> -protected:
> - FileSystem &m_fs;
> };
>
> class StandardTildeExpressionResolver : public TildeExpressionResolver {
> public:
> - StandardTildeExpressionResolver(FileSystem &fs)
> - : TildeExpressionResolver(fs) {}
> -
> bool ResolveExact(llvm::StringRef Expr,
> llvm::SmallVectorImpl<char> &Output) override;
> bool ResolvePartial(llvm::StringRef Expr, llvm::StringSet<> &Output) override;
> };
> -} // namespace lldb_private
> +}
>
> #endif // #ifndef LLDB_UTILITY_TILDE_EXPRESSION_RESOLVER_H
>
> Modified: lldb/trunk/source/Commands/CommandCompletions.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Commands/CommandCompletions.cpp?rev=346466&r1=346465&r2=346466&view=diff
> ==============================================================================
> --- lldb/trunk/source/Commands/CommandCompletions.cpp (original)
> +++ lldb/trunk/source/Commands/CommandCompletions.cpp Thu Nov 8 17:59:28 2018
> @@ -231,7 +231,7 @@ static int DiskFilesOrDirectories(const
> static int DiskFilesOrDirectories(CompletionRequest &request,
> bool only_directories) {
> request.SetWordComplete(false);
> - StandardTildeExpressionResolver resolver(FileSystem::Instance());
> + StandardTildeExpressionResolver resolver;
> StringList matches;
> DiskFilesOrDirectories(request.GetCursorArgumentPrefix(), only_directories,
> matches, resolver);
>
> Modified: lldb/trunk/source/Host/common/FileSystem.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/common/FileSystem.cpp?rev=346466&r1=346465&r2=346466&view=diff
> ==============================================================================
> --- lldb/trunk/source/Host/common/FileSystem.cpp (original)
> +++ lldb/trunk/source/Host/common/FileSystem.cpp Thu Nov 8 17:59:28 2018
> @@ -183,9 +183,8 @@ std::error_code FileSystem::MakeAbsolute
> }
>
> std::error_code FileSystem::GetRealPath(const Twine &path,
> - SmallVectorImpl<char> &output,
> - bool expand_tilde) const {
> - return m_fs->getRealPath(path, output, expand_tilde);
> + SmallVectorImpl<char> &output) const {
> + return m_fs->getRealPath(path, output);
> }
>
> void FileSystem::Resolve(SmallVectorImpl<char> &path) {
> @@ -194,8 +193,8 @@ void FileSystem::Resolve(SmallVectorImpl
>
> // Resolve tilde.
> SmallString<128> original_path(path.begin(), path.end());
> - StandardTildeExpressionResolver resolver(*this);
> - resolver.ResolveFullPath(original_path, path);
> + StandardTildeExpressionResolver Resolver;
> + Resolver.ResolveFullPath(original_path, path);
>
> // Try making the path absolute if it exists.
> SmallString<128> absolute_path(path.begin(), path.end());
>
> Modified: lldb/trunk/source/Target/TargetList.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/TargetList.cpp?rev=346466&r1=346465&r2=346466&view=diff
> ==============================================================================
> --- lldb/trunk/source/Target/TargetList.cpp (original)
> +++ lldb/trunk/source/Target/TargetList.cpp Thu Nov 8 17:59:28 2018
> @@ -351,8 +351,8 @@ Status TargetList::CreateTargetInternal(
> // we want to expand the tilde but we don't want to resolve any symbolic
> // links so we can't use the FileSpec constructor's resolve flag
> llvm::SmallString<64> unglobbed_path;
> - StandardTildeExpressionResolver resolver(FileSystem::Instance());
> - resolver.ResolveFullPath(user_exe_path, unglobbed_path);
> + StandardTildeExpressionResolver Resolver;
> + Resolver.ResolveFullPath(user_exe_path, unglobbed_path);
>
> if (unglobbed_path.empty())
> file = FileSpec(user_exe_path);
>
> Modified: lldb/trunk/source/Utility/TildeExpressionResolver.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/TildeExpressionResolver.cpp?rev=346466&r1=346465&r2=346466&view=diff
> ==============================================================================
> --- lldb/trunk/source/Utility/TildeExpressionResolver.cpp (original)
> +++ lldb/trunk/source/Utility/TildeExpressionResolver.cpp Thu Nov 8 17:59:28 2018
> @@ -8,13 +8,13 @@
> //===----------------------------------------------------------------------===//
>
> #include "lldb/Utility/TildeExpressionResolver.h"
> -#include "lldb/Host/FileSystem.h"
>
> #include <assert.h> // for assert
> #include <system_error> // for error_code
>
> #include "llvm/ADT/STLExtras.h" // for any_of
> #include "llvm/ADT/SmallVector.h" // for SmallVectorImpl
> +#include "llvm/Support/FileSystem.h"
> #include "llvm/Support/Path.h"
> #include "llvm/Support/raw_ostream.h" // for fs
>
> @@ -37,7 +37,7 @@ bool StandardTildeExpressionResolver::Re
> assert(!llvm::any_of(Expr, [](char c) { return path::is_separator(c); }));
> assert(Expr.empty() || Expr[0] == '~');
>
> - return !m_fs.GetRealPath(Expr, Output, true);
> + return !fs::real_path(Expr, Output, true);
> }
>
> bool StandardTildeExpressionResolver::ResolvePartial(StringRef Expr,
>
> Modified: lldb/trunk/unittests/Host/FileSystemTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Host/FileSystemTest.cpp?rev=346466&r1=346465&r2=346466&view=diff
> ==============================================================================
> --- lldb/trunk/unittests/Host/FileSystemTest.cpp (original)
> +++ lldb/trunk/unittests/Host/FileSystemTest.cpp Thu Nov 8 17:59:28 2018
> @@ -69,8 +69,8 @@ public:
> return std::error_code();
> }
> // Map any symlink to "/symlink".
> - std::error_code getRealPath(const Twine &Path, SmallVectorImpl<char> &Output,
> - bool ExpandTilde) const override {
> + std::error_code getRealPath(const Twine &Path,
> + SmallVectorImpl<char> &Output) const override {
> auto I = FilesAndDirs.find(Path.str());
> if (I == FilesAndDirs.end())
> return make_error_code(llvm::errc::no_such_file_or_directory);
>
> Modified: lldb/trunk/unittests/Interpreter/TestCompletion.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Interpreter/TestCompletion.cpp?rev=346466&r1=346465&r2=346466&view=diff
> ==============================================================================
> --- lldb/trunk/unittests/Interpreter/TestCompletion.cpp (original)
> +++ lldb/trunk/unittests/Interpreter/TestCompletion.cpp Thu Nov 8 17:59:28 2018
> @@ -7,11 +7,9 @@
> //
> //===----------------------------------------------------------------------===//
>
> -#include "lldb/Host/FileSystem.h"
> #include "lldb/Interpreter/CommandCompletions.h"
> #include "lldb/Utility/StringList.h"
> #include "lldb/Utility/TildeExpressionResolver.h"
> -
> #include "gmock/gmock.h"
> #include "gtest/gtest.h"
>
> @@ -67,8 +65,6 @@ protected:
> SmallString<128> FileBaz;
>
> void SetUp() override {
> - FileSystem::Initialize();
> -
> // chdir back into the original working dir this test binary started with.
> // A previous test may have have changed the working dir.
> ASSERT_NO_ERROR(fs::set_current_path(OriginalWorkingDir));
> @@ -109,10 +105,7 @@ protected:
> ASSERT_NO_ERROR(fs::current_path(OriginalWorkingDir));
> }
>
> - void TearDown() override {
> - ASSERT_NO_ERROR(fs::remove_directories(BaseDir));
> - FileSystem::Terminate();
> - }
> + void TearDown() override { ASSERT_NO_ERROR(fs::remove_directories(BaseDir)); }
>
> static bool HasEquivalentFile(const Twine &Path, const StringList &Paths) {
> for (size_t I = 0; I < Paths.GetSize(); ++I) {
> @@ -165,7 +158,7 @@ TEST_F(CompletionTest, DirCompletionAbso
>
> std::string Prefixes[] = {(Twine(BaseDir) + "/").str(), ""};
>
> - StandardTildeExpressionResolver Resolver(FileSystem::Instance());
> + StandardTildeExpressionResolver Resolver;
> StringList Results;
>
> // When a directory is specified that doesn't end in a slash, it searches
> @@ -204,7 +197,7 @@ TEST_F(CompletionTest, FileCompletionAbs
> // all check this by asserting an exact result count, and verifying against
> // known folders.
>
> - StandardTildeExpressionResolver Resolver(FileSystem::Instance());
> + StandardTildeExpressionResolver Resolver;
> StringList Results;
> // When an item is specified that doesn't end in a slash but exactly matches
> // one item, it returns that item.
> @@ -255,8 +248,7 @@ TEST_F(CompletionTest, FileCompletionAbs
> }
>
> TEST_F(CompletionTest, DirCompletionUsername) {
> - MockTildeExpressionResolver Resolver(FileSystem::Instance(), "James",
> - BaseDir);
> + MockTildeExpressionResolver Resolver("James", BaseDir);
> Resolver.AddKnownUser("Kirk", DirFooB);
> Resolver.AddKnownUser("Lars", DirFooC);
> Resolver.AddKnownUser("Jason", DirFoo);
>
> Modified: lldb/trunk/unittests/TestingSupport/MockTildeExpressionResolver.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/TestingSupport/MockTildeExpressionResolver.cpp?rev=346466&r1=346465&r2=346466&view=diff
> ==============================================================================
> --- lldb/trunk/unittests/TestingSupport/MockTildeExpressionResolver.cpp (original)
> +++ lldb/trunk/unittests/TestingSupport/MockTildeExpressionResolver.cpp Thu Nov 8 17:59:28 2018
> @@ -13,10 +13,9 @@
> using namespace lldb_private;
> using namespace llvm;
>
> -MockTildeExpressionResolver::MockTildeExpressionResolver(FileSystem &fs,
> - StringRef CurrentUser,
> +MockTildeExpressionResolver::MockTildeExpressionResolver(StringRef CurrentUser,
> StringRef HomeDir)
> - : TildeExpressionResolver(fs), CurrentUser(CurrentUser) {
> + : CurrentUser(CurrentUser) {
> UserDirectories.insert(std::make_pair(CurrentUser, HomeDir));
> }
>
>
> Modified: lldb/trunk/unittests/TestingSupport/MockTildeExpressionResolver.h
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/TestingSupport/MockTildeExpressionResolver.h?rev=346466&r1=346465&r2=346466&view=diff
> ==============================================================================
> --- lldb/trunk/unittests/TestingSupport/MockTildeExpressionResolver.h (original)
> +++ lldb/trunk/unittests/TestingSupport/MockTildeExpressionResolver.h Thu Nov 8 17:59:28 2018
> @@ -16,13 +16,12 @@
> #include "llvm/ADT/StringMap.h"
>
> namespace lldb_private {
> -class FileSystem;
> class MockTildeExpressionResolver : public TildeExpressionResolver {
> llvm::StringRef CurrentUser;
> llvm::StringMap<llvm::StringRef> UserDirectories;
>
> public:
> - MockTildeExpressionResolver(FileSystem &fs, llvm::StringRef CurrentUser,
> + MockTildeExpressionResolver(llvm::StringRef CurrentUser,
> llvm::StringRef HomeDir);
>
> void AddKnownUser(llvm::StringRef User, llvm::StringRef HomeDir);
>
> Modified: lldb/trunk/unittests/Utility/TildeExpressionResolverTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/TildeExpressionResolverTest.cpp?rev=346466&r1=346465&r2=346466&view=diff
> ==============================================================================
> --- lldb/trunk/unittests/Utility/TildeExpressionResolverTest.cpp (original)
> +++ lldb/trunk/unittests/Utility/TildeExpressionResolverTest.cpp Thu Nov 8 17:59:28 2018
> @@ -1,7 +1,6 @@
> #include "gtest/gtest.h"
>
> #include "TestingSupport/MockTildeExpressionResolver.h"
> -#include "lldb/Host/FileSystem.h"
> #include "lldb/Utility/TildeExpressionResolver.h"
>
> #include "llvm/ADT/SmallString.h"
> @@ -10,8 +9,7 @@ using namespace llvm;
> using namespace lldb_private;
>
> TEST(TildeExpressionResolver, ResolveFullPath) {
> - FileSystem fs;
> - MockTildeExpressionResolver Resolver(fs, "James", "/james");
> + MockTildeExpressionResolver Resolver("James", "/james");
> Resolver.AddKnownUser("Kirk", "/kirk");
> Resolver.AddKnownUser("Lars", "/lars");
> Resolver.AddKnownUser("Jason", "/jason");
>
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
More information about the lldb-commits
mailing list