Refactoring TTerminal::ReadFile + Adding TTerminal::TryReadFile + Solving memory leak on the way

Source commit: 4a5c5634006d2bd9a1c6c4831d0e528b91ecd9ae

source/core/ScpFileSystem.cpp

@@ -1048,10 +1048,7 @@ void __fastcall TSCPFileSystem::ReadDirectory(TRemoteFileList * FileList)
         {
           // Empty file list -> probably "permission denied", we
           // at least get link to parent directory ("..")
-          TRemoteFile * File;
-          FTerminal->ReadFile(
-            UnixIncludeTrailingBackslash(FTerminal->FFiles->Directory) +
-              PARENTDIRECTORY, File);
+          TRemoteFile * File = FTerminal->ReadFile(UnixCombinePaths(FTerminal->FFiles->Directory, PARENTDIRECTORY));
           Empty = (File == NULL);
           if (!Empty)
           {

source/core/Script.cpp

@@ -658,7 +658,7 @@ TStrings * __fastcall TScript::CreateFileList(TScriptProcParams * Parameters, in
             FTerminal->ExceptionOnFail = true;
             try
             {
-              FTerminal->ReadFile(UnixExcludeTrailingBackslash(FileName), File);
+              File = FTerminal->ReadFile(UnixExcludeTrailingBackslash(FileName));
               if (!File->HaveFullFileName)
               {
                 File->FullFileName = FileName;

source/core/SftpFileSystem.cpp

@@ -3610,9 +3610,7 @@ void __fastcall TSFTPFileSystem::ReadDirectory(TRemoteFileList * FileList)
           FTerminal->ExceptionOnFail = true;
           try
           {
-            File = NULL;
-            FTerminal->ReadFile(
-              UnixIncludeTrailingBackslash(FileList->Directory) + PARENTDIRECTORY, File);
+            File = FTerminal->ReadFile(UnixCombinePaths(FileList->Directory, PARENTDIRECTORY));
           }
           __finally
           {

source/core/Terminal.cpp

@@ -3579,7 +3579,7 @@ TRemoteFile * __fastcall TTerminal::ReadFileListing(UnicodeString Path)
     {
       // reset caches
       AnnounceFileListOperation();
-      ReadFile(Path, File);
+      File = ReadFile(Path);
       Action.File(File);
     }
     catch(Exception & E)
@@ -3755,64 +3755,60 @@ void __fastcall TTerminal::ReadSymlink(TRemoteFile * SymlinkFile,
   }
 }
 //---------------------------------------------------------------------------
-void __fastcall TTerminal::ReadFile(const UnicodeString FileName,
-  TRemoteFile *& File)
+TRemoteFile * TTerminal::ReadFile(const UnicodeString & FileName)
 {
   DebugAssert(FFileSystem);
-  File = NULL;
+  std::unique_ptr<TRemoteFile> File;
   try
   {
     LogEvent(FORMAT(L"Listing file \"%s\".", (FileName)));
-    FFileSystem->ReadFile(FileName, File);
+    TRemoteFile * AFile = NULL;
+    FFileSystem->ReadFile(FileName, AFile);
+    File.reset(AFile);
     ReactOnCommand(fsListFile);
-    LogRemoteFile(File);
+    LogRemoteFile(File.get());
   }
   catch (Exception &E)
   {
-    if (File) delete File;
-    File = NULL;
+    File.reset(NULL);
     CommandError(&E, FMTLOAD(CANT_GET_ATTRS, (FileName)));
   }
+  return File.release();
 }
 //---------------------------------------------------------------------------
-bool __fastcall TTerminal::FileExists(const UnicodeString FileName, TRemoteFile ** AFile)
+TRemoteFile * TTerminal::TryReadFile(const UnicodeString & FileName)
 {
-  bool Result;
-  TRemoteFile * File = NULL;
+  TRemoteFile * File;
   try
   {
     ExceptionOnFail = true;
     try
     {
-      ReadFile(UnixExcludeTrailingBackslash(FileName), File);
+      File = ReadFile(UnixExcludeTrailingBackslash(FileName));
     }
     __finally
     {
       ExceptionOnFail = false;
     }
-
-    if (AFile != NULL)
-    {
-      *AFile = File;
-    }
-    else
-    {
-      delete File;
-    }
-    Result = true;
   }
   catch(...)
   {
     if (Active)
     {
-      Result = false;
+      File = NULL;
     }
     else
     {
       throw;
     }
   }
-  return Result;
+  return File;
+}
+//---------------------------------------------------------------------------
+bool TTerminal::FileExists(const UnicodeString & FileName)
+{
+  std::unique_ptr<TRemoteFile> File(TryReadFile(FileName));
+  return (File.get() != NULL);
 }
 //---------------------------------------------------------------------------
 void __fastcall TTerminal::AnnounceFileListOperation()
@@ -4645,8 +4641,7 @@ bool __fastcall TTerminal::DoRenameFile(
   UnicodeString AbsoluteNewName = AbsolutePath(NewName, true);
   bool Result = true;
   bool ExistenceKnown = false;
-  TRemoteFile * DuplicateFile = NULL;
-  std::unique_ptr<TRemoteFile> DuplicateFileOwner(DuplicateFile);
+  std::unique_ptr<TRemoteFile> DuplicateFile;
   if (BatchOverwrite == boNone)
   {
     DebugAssert(!DontOverwrite); // unsupported combination
@@ -4661,11 +4656,10 @@ bool __fastcall TTerminal::DoRenameFile(
            Configuration->ConfirmOverwriting &&
            !DontOverwrite)
   {
-    FileExists(AbsoluteNewName, &DuplicateFile);
-    DuplicateFileOwner.reset(DuplicateFile);
+    DuplicateFile.reset(TryReadFile(AbsoluteNewName));
     ExistenceKnown = true;
 
-    if (DuplicateFile != NULL)
+    if (DuplicateFile.get() != NULL)
     {
       UnicodeString QuestionFmt;
       if (DuplicateFile->IsDirectory)
@@ -4734,13 +4728,12 @@ bool __fastcall TTerminal::DoRenameFile(
       {
         if (!ExistenceKnown)
         {
-          FileExists(AbsoluteNewName, &DuplicateFile);
-          DuplicateFileOwner.reset(DuplicateFile);
+          DuplicateFile.reset(TryReadFile(AbsoluteNewName));
         }
 
-        if (DuplicateFile != NULL)
+        if (DuplicateFile.get() != NULL)
         {
-          DoDeleteFile(AbsoluteNewName, DuplicateFile, 0);
+          DoDeleteFile(AbsoluteNewName, DuplicateFile.get(), 0);
         }
       }
 
@@ -7251,11 +7244,11 @@ void __fastcall TTerminal::SourceRobust(
 bool __fastcall TTerminal::CreateTargetDirectory(
   const UnicodeString & DirectoryPath, int Attrs, const TCopyParamType * CopyParam)
 {
-  TRemoteFile * File = NULL;
+  std::unique_ptr<TRemoteFile> File(TryReadFile(DirectoryPath));
   bool DoCreate =
-    !FileExists(DirectoryPath, &File) ||
+    (File.get() == NULL) ||
     !File->IsDirectory; // just try to create and make it fail
-  delete File;
+  File.reset(NULL);
   if (DoCreate)
   {
     TRemoteProperties Properties;
@@ -8403,13 +8396,11 @@ UnicodeString __fastcall TTerminal::DecryptFileName(const UnicodeString & Path,
 //---------------------------------------------------------------------------
 TRemoteFile * TTerminal::CheckRights(const UnicodeString & EntryType, const UnicodeString & FileName, bool & WrongRights)
 {
-  std::unique_ptr<TRemoteFile> FileOwner;
-  TRemoteFile * File;
+  std::unique_ptr<TRemoteFile> File;
   try
   {
     LogEvent(FORMAT(L"Checking %s \"%s\"...", (LowerCase(EntryType), FileName)));
-    ReadFile(FileName, File);
-    FileOwner.reset(File);
+    File.reset(ReadFile(FileName));
     int ForbiddenRights = TRights::rfGroupWrite | TRights::rfOtherWrite;
     if ((File->Rights->Number & ForbiddenRights) != 0)
     {
@@ -8424,7 +8415,7 @@ TRemoteFile * TTerminal::CheckRights(const UnicodeString & EntryType, const Unic
   catch (Exception & E)
   {
   }
-  return FileOwner.release();
+  return File.release();
 }
 //---------------------------------------------------------------------------
 UnicodeString TTerminal::UploadPublicKey(const UnicodeString & FileName)

source/core/Terminal.h

@@ -527,8 +527,9 @@ public:
   TRemoteFileList * __fastcall ReadDirectoryListing(UnicodeString Directory, const TFileMasks & Mask);
   TRemoteFileList * __fastcall CustomReadDirectoryListing(UnicodeString Directory, bool UseCache);
   TRemoteFile * __fastcall ReadFileListing(UnicodeString Path);
-  void __fastcall ReadFile(const UnicodeString FileName, TRemoteFile *& File);
-  bool __fastcall FileExists(const UnicodeString FileName, TRemoteFile ** File = NULL);
+  TRemoteFile * ReadFile(const UnicodeString & FileName);
+  TRemoteFile * TryReadFile(const UnicodeString & FileName);
+  bool FileExists(const UnicodeString & FileName);
   void __fastcall ReadSymlink(TRemoteFile * SymlinkFile, TRemoteFile *& File);
   bool __fastcall CopyToLocal(
     TStrings * FilesToCopy, const UnicodeString & TargetDir, const TCopyParamType * CopyParam, int Params,

source/forms/CustomScpExplorer.cpp

@@ -3254,20 +3254,14 @@ void __fastcall TCustomScpExplorerForm::EditNew(TOperationSide Side)
       {
         Name = AbsolutePath(Terminal->CurrentDirectory, Name);
 
-        TRemoteFile * File = NULL;
-        if (Terminal->FileExists(Name, &File) &&
+        std::unique_ptr<TRemoteFile> File(Terminal->TryReadFile(Name));
+        if ((File.get() != NULL) &&
             !File->IsDirectory)
         {
-          try
-          {
-            ExecuteRemoteFile(Name, File, efDefaultEditor);
-            ExistingFile = true;
-          }
-          __finally
-          {
-            delete File;
-          }
+          ExecuteRemoteFile(Name, File.get(), efDefaultEditor);
+          ExistingFile = true;
         }
+        File.reset(NULL);
 
         if (!ExistingFile)
         {
@@ -3837,16 +3831,14 @@ void __fastcall TCustomScpExplorerForm::ExecuteFile(TOperationSide Side,
         TDateTimePrecision Precision;
         MaskParams.Modification = DView->ItemFileTime(Item, Precision);
 
-        std::unique_ptr<TRemoteFile> FileOwner;
+        std::unique_ptr<TRemoteFile> File;
         if (!IsSideLocalBrowser(Side))
         {
-          TRemoteFile * File = NULL;
-          Terminal->FileExists(FullFileName, &File);
-          if (File != NULL)
+          File.reset(Terminal->TryReadFile(FullFileName));
+          if (File.get() != NULL)
           {
             File->FullFileName = FullFileName;
-            Object = File;
-            FileOwner.reset(File);
+            Object = File.get();
             MaskParams.Size = File->Size;
             MaskParams.Modification = File->Modification;
           }
@@ -4011,9 +4003,7 @@ void __fastcall TCustomScpExplorerForm::ExecutedFileReload(
     Terminal->ExceptionOnFail = true;
     try
     {
-      TRemoteFile * AFile = NULL;
-      Terminal->ReadFile(RemoteFileName, AFile);
-      File.reset(AFile);
+      File.reset(Terminal->ReadFile(RemoteFileName));
       if (!File->HaveFullFileName)
       {
         File->FullFileName = RemoteFileName;
@@ -6433,13 +6423,10 @@ void __fastcall TCustomScpExplorerForm::StandaloneEdit(const UnicodeString & Fil
 {
   UnicodeString FullFileName = AbsolutePath(Terminal->CurrentDirectory, FileName);
 
-  TRemoteFile * File = NULL;
-  Terminal->ReadFile(FullFileName, File);
-  if (File != NULL)
+  std::unique_ptr<TRemoteFile> File(Terminal->ReadFile(FullFileName));
+  if (File.get() != NULL)
   {
-    std::unique_ptr<TRemoteFile> FileOwner(File);
-
-    ExecuteRemoteFile(FullFileName, File, efInternalEditor);
+    ExecuteRemoteFile(FullFileName, File.get(), efInternalEditor);
 
     Application->ShowMainForm = false;
 

source/windows/WinMain.cpp

@@ -130,7 +130,7 @@ void __fastcall Download(TTerminal * Terminal, const UnicodeString FileName, int
     Terminal->ExceptionOnFail = true;
     try
     {
-      Terminal->ReadFile(FileName, File);
+      File = Terminal->ReadFile(FileName);
     }
     __finally
     {