Remove Release() from IFXCRT_FileAccess (issue 1994323002 by tsepez@chromium.org)

0 views
Skip to first unread message

tse...@chromium.org

unread,
May 19, 2016, 6:10:02 PM5/19/16
to the...@chromium.org, pdfium-...@googlegroups.com
Reviewers: Lei Zhang
CL: https://codereview.chromium.org/1994323002/

Message:
Lei, for review.

Description:
Remove Release() from IFXCRT_FileAccess

Base URL: https://pdfium.googlesource.com/pdfium.git@master

Affected files (+27, -36 lines):
M core/fxcrt/extension.h
M core/fxcrt/fx_extension.cpp
M core/fxcrt/fxcrt_posix.h
M core/fxcrt/fxcrt_posix.cpp
M core/fxcrt/fxcrt_windows.h
M core/fxcrt/fxcrt_windows.cpp


Index: core/fxcrt/extension.h
diff --git a/core/fxcrt/extension.h b/core/fxcrt/extension.h
index b0e7b9e117ef519b65524001eb00cbc2ea3f5c7f..848ab168883a8624918b7a3c41c4226f10f67699 100644
--- a/core/fxcrt/extension.h
+++ b/core/fxcrt/extension.h
@@ -8,17 +8,19 @@
#define CORE_FXCRT_EXTENSION_H_

#include <algorithm>
+#include <memory>

#include "core/fxcrt/include/fx_basic.h"
#include "core/fxcrt/include/fx_safe_types.h"

class IFXCRT_FileAccess {
public:
+ static IFXCRT_FileAccess* Create();
virtual ~IFXCRT_FileAccess() {}
+
virtual FX_BOOL Open(const CFX_ByteStringC& fileName, uint32_t dwMode) = 0;
virtual FX_BOOL Open(const CFX_WideStringC& fileName, uint32_t dwMode) = 0;
virtual void Close() = 0;
- virtual void Release() = 0;
virtual FX_FILESIZE GetSize() const = 0;
virtual FX_FILESIZE GetPosition() const = 0;
virtual FX_FILESIZE SetPosition(FX_FILESIZE pos) = 0;
@@ -31,7 +33,6 @@ class IFXCRT_FileAccess {
virtual FX_BOOL Flush() = 0;
virtual FX_BOOL Truncate(FX_FILESIZE szFile) = 0;
};
-IFXCRT_FileAccess* FXCRT_FileAccess_Create();

#ifdef PDF_ENABLE_XFA
class CFX_CRTFileAccess : public IFX_FileAccess {
@@ -86,7 +87,7 @@ class CFX_CRTFileStream final : public IFX_FileStream {
FX_BOOL Flush() override;

protected:
- IFXCRT_FileAccess* m_pFile;
+ std::unique_ptr<IFXCRT_FileAccess> m_pFile;
uint32_t m_dwCount;
};

Index: core/fxcrt/fx_extension.cpp
diff --git a/core/fxcrt/fx_extension.cpp b/core/fxcrt/fx_extension.cpp
index 1dddf09fd677a2a581ceab77d253846a3831fc5b..3717e16207c8a43bce0b301ddc2ac1d45b6d0353 100644
--- a/core/fxcrt/fx_extension.cpp
+++ b/core/fxcrt/fx_extension.cpp
@@ -17,11 +17,7 @@
CFX_CRTFileStream::CFX_CRTFileStream(IFXCRT_FileAccess* pFA)
: m_pFile(pFA), m_dwCount(1) {}

-CFX_CRTFileStream::~CFX_CRTFileStream() {
- if (m_pFile) {
- m_pFile->Release();
- }
-}
+CFX_CRTFileStream::~CFX_CRTFileStream() {}

IFX_FileStream* CFX_CRTFileStream::Retain() {
m_dwCount++;
@@ -83,27 +79,22 @@ IFX_FileAccess* FX_CreateDefaultFileAccess(const CFX_WideStringC& wsPath) {
#endif // PDF_ENABLE_XFA

IFX_FileStream* FX_CreateFileStream(const FX_CHAR* filename, uint32_t dwModes) {
- IFXCRT_FileAccess* pFA = FXCRT_FileAccess_Create();
- if (!pFA) {
- return NULL;
- }
- if (!pFA->Open(filename, dwModes)) {
- pFA->Release();
- return NULL;
- }
- return new CFX_CRTFileStream(pFA);
+ std::unique_ptr<IFXCRT_FileAccess> pFA(IFXCRT_FileAccess::Create());
+ if (!pFA)
+ return nullptr;
+ if (!pFA->Open(filename, dwModes))
+ return nullptr;
+ return new CFX_CRTFileStream(pFA.release());
}
+
IFX_FileStream* FX_CreateFileStream(const FX_WCHAR* filename,
uint32_t dwModes) {
- IFXCRT_FileAccess* pFA = FXCRT_FileAccess_Create();
- if (!pFA) {
- return NULL;
- }
- if (!pFA->Open(filename, dwModes)) {
- pFA->Release();
- return NULL;
- }
- return new CFX_CRTFileStream(pFA);
+ std::unique_ptr<IFXCRT_FileAccess> pFA(IFXCRT_FileAccess::Create());
+ if (!pFA)
+ return nullptr;
+ if (!pFA->Open(filename, dwModes))
+ return nullptr;
+ return new CFX_CRTFileStream(pFA.release());
}
IFX_FileRead* FX_CreateFileRead(const FX_CHAR* filename) {
return FX_CreateFileStream(filename, FX_FILEMODE_ReadOnly);
Index: core/fxcrt/fxcrt_posix.cpp
diff --git a/core/fxcrt/fxcrt_posix.cpp b/core/fxcrt/fxcrt_posix.cpp
index 9237acef6bf50d04d6d0a6451dd73a04da8f2aae..053f89c2c9192de0f075953edf420dc391b3c6a9 100644
--- a/core/fxcrt/fxcrt_posix.cpp
+++ b/core/fxcrt/fxcrt_posix.cpp
@@ -11,9 +11,12 @@
#if _FXM_PLATFORM_ == _FXM_PLATFORM_LINUX_ || \
_FXM_PLATFORM_ == _FXM_PLATFORM_APPLE_ || \
_FXM_PLATFORM_ == _FXM_PLATFORM_ANDROID_
-IFXCRT_FileAccess* FXCRT_FileAccess_Create() {
+
+// static
+IFXCRT_FileAccess* IFXCRT_FileAccess::Create() {
return new CFXCRT_FileAccess_Posix;
}
+
void FXCRT_Posix_GetFileMode(uint32_t dwModes,
int32_t& nFlags,
int32_t& nMasks) {
@@ -54,9 +57,6 @@ void CFXCRT_FileAccess_Posix::Close() {
close(m_nFD);
m_nFD = -1;
}
-void CFXCRT_FileAccess_Posix::Release() {
- delete this;
-}
FX_FILESIZE CFXCRT_FileAccess_Posix::GetSize() const {
if (m_nFD < 0) {
return 0;
@@ -127,4 +127,5 @@ FX_BOOL CFXCRT_FileAccess_Posix::Truncate(FX_FILESIZE szFile) {
}
return !ftruncate(m_nFD, szFile);
}
+
#endif
Index: core/fxcrt/fxcrt_posix.h
diff --git a/core/fxcrt/fxcrt_posix.h b/core/fxcrt/fxcrt_posix.h
index 969707a7b98a8e535bff054a40557a808d48532d..37f6548d4019b31d858ea63667f47c9be6ee9dbf 100644
--- a/core/fxcrt/fxcrt_posix.h
+++ b/core/fxcrt/fxcrt_posix.h
@@ -21,7 +21,6 @@ class CFXCRT_FileAccess_Posix : public IFXCRT_FileAccess {
FX_BOOL Open(const CFX_ByteStringC& fileName, uint32_t dwMode) override;
FX_BOOL Open(const CFX_WideStringC& fileName, uint32_t dwMode) override;
void Close() override;
- void Release() override;
FX_FILESIZE GetSize() const override;
FX_FILESIZE GetPosition() const override;
FX_FILESIZE SetPosition(FX_FILESIZE pos) override;
Index: core/fxcrt/fxcrt_windows.cpp
diff --git a/core/fxcrt/fxcrt_windows.cpp b/core/fxcrt/fxcrt_windows.cpp
index eb584ca804c4c113523e87d23b1f713ccfe0dfc6..d4b4e50c2c24cc5bcfc8052ad608e060f9881f65 100644
--- a/core/fxcrt/fxcrt_windows.cpp
+++ b/core/fxcrt/fxcrt_windows.cpp
@@ -9,9 +9,12 @@
#include "core/fxcrt/include/fx_string.h"

#if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_
-IFXCRT_FileAccess* FXCRT_FileAccess_Create() {
+
+// static
+IFXCRT_FileAccess* IFXCRT_FileAccess::Create() {
return new CFXCRT_FileAccess_Win64;
}
+
void FXCRT_Windows_GetFileMode(uint32_t dwMode,
uint32_t& dwAccess,
uint32_t& dwShare,
@@ -75,9 +78,6 @@ void CFXCRT_FileAccess_Win64::Close() {
::CloseHandle(m_hFile);
m_hFile = NULL;
}
-void CFXCRT_FileAccess_Win64::Release() {
- delete this;
-}
FX_FILESIZE CFXCRT_FileAccess_Win64::GetSize() const {
if (!m_hFile) {
return 0;
Index: core/fxcrt/fxcrt_windows.h
diff --git a/core/fxcrt/fxcrt_windows.h b/core/fxcrt/fxcrt_windows.h
index 7444446d454a1aacdae53eee83fd6aa5cd1b1c6e..19cef0adc26591ee66b151d5d9298e5bb307b6a2 100644
--- a/core/fxcrt/fxcrt_windows.h
+++ b/core/fxcrt/fxcrt_windows.h
@@ -19,7 +19,6 @@ class CFXCRT_FileAccess_Win64 : public IFXCRT_FileAccess {
FX_BOOL Open(const CFX_ByteStringC& fileName, uint32_t dwMode) override;
FX_BOOL Open(const CFX_WideStringC& fileName, uint32_t dwMode) override;
void Close() override;
- void Release() override;
FX_FILESIZE GetSize() const override;
FX_FILESIZE GetPosition() const override;
FX_FILESIZE SetPosition(FX_FILESIZE pos) override;


the...@chromium.org

unread,
May 19, 2016, 6:50:05 PM5/19/16
to tse...@chromium.org, pdfium-...@googlegroups.com
There's another FXCRT_FileAccess_Create() impl in
core/fxcrt/fxcrt_platforms.cpp, but it's not obvious who is using it.


https://codereview.chromium.org/1994323002/diff/1/core/fxcrt/fx_extension.cpp
File core/fxcrt/fx_extension.cpp (right):

https://codereview.chromium.org/1994323002/diff/1/core/fxcrt/fx_extension.cpp#newcode83
core/fxcrt/fx_extension.cpp:83: if (!pFA)
IFXCRT_FileAccess::Create() can't fail.

https://codereview.chromium.org/1994323002/diff/1/core/fxcrt/fx_extension.cpp#newcode87
core/fxcrt/fx_extension.cpp:87: return new
CFX_CRTFileStream(pFA.release());
CFX_CRTFileStream should take a unique_ptr. The only 2 callers are here
and below according to Code Search.

https://codereview.chromium.org/1994323002/

tse...@chromium.org

unread,
May 19, 2016, 7:05:59 PM5/19/16
to the...@chromium.org, pdfium-...@googlegroups.com
Guarded by
#if (_FXM_PLATFORM_ != _FXM_PLATFORM_WINDOWS_ && \
_FXM_PLATFORM_ != _FXM_PLATFORM_LINUX_ && \
_FXM_PLATFORM_ != _FXM_PLATFORM_APPLE_ && \
_FXM_PLATFORM_ != _FXM_PLATFORM_ANDROID_)

and yet in fx_system.h, the only values are:

// _FXM_PLATFORM_ values;

#define _FXM_PLATFORM_WINDOWS_ 1 // _FX_WIN32_DESKTOP_ or _FX_WIN64_DESKTOP_.

#define _FXM_PLATFORM_LINUX_ 2 // _FX_LINUX_DESKTOP_ always.

#define _FXM_PLATFORM_APPLE_ 3 // _FX_MACOSX_ always.

#define _FXM_PLATFORM_ANDROID_ 4 // _FX_ANDROID_ always.


So I would conclude "nobody".





https://codereview.chromium.org/1994323002/

tse...@chromium.org

unread,
May 19, 2016, 7:18:31 PM5/19/16
to the...@chromium.org, pdfium-...@googlegroups.com
On 2016/05/19 22:50:05, Lei Zhang wrote:
> IFXCRT_FileAccess::Create() can't fail.

Done.


https://codereview.chromium.org/1994323002/diff/1/core/fxcrt/fx_extension.cpp#newcode87
core/fxcrt/fx_extension.cpp:87: return new
CFX_CRTFileStream(pFA.release());
On 2016/05/19 22:50:05, Lei Zhang wrote:
> CFX_CRTFileStream should take a unique_ptr. The only 2 callers are
here and
> below according to Code Search.

the...@chromium.org

unread,
May 19, 2016, 7:59:03 PM5/19/16
to tse...@chromium.org, pdfium-...@googlegroups.com

tse...@chromium.org

unread,
May 19, 2016, 8:13:53 PM5/19/16
to the...@chromium.org, pdfium-...@googlegroups.com

https://codereview.chromium.org/1994323002/diff/40001/core/fxcrt/fx_extension.cpp
File core/fxcrt/fx_extension.cpp (right):

https://codereview.chromium.org/1994323002/diff/40001/core/fxcrt/fx_extension.cpp#newcode18
core/fxcrt/fx_extension.cpp:18: : m_pFile(std::move(pFA)), m_dwCount(1)
{}
On 2016/05/19 23:59:02, Lei Zhang wrote:
> #include <utility>

Done.

https://codereview.chromium.org/1994323002/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 19, 2016, 8:14:04 PM5/19/16
to tse...@chromium.org, the...@chromium.org, commi...@chromium.org, pdfium-...@googlegroups.com

commit-bot@chromium.org via codereview.chromium.org

unread,
May 19, 2016, 8:31:06 PM5/19/16
to tse...@chromium.org, the...@chromium.org, commi...@chromium.org, pdfium-...@googlegroups.com
Reply all
Reply to author
Forward
0 new messages