Add FileUtils::getContents(). #15479

xpol · 2016-04-21T05:37:03Z

Why another file read API?

Current APIs has some problem:

FileUtils::getDataFormFile() can't return string, convert from Data to string will make extra copy.
FileUtils::getStringFormFile() can't read binary file, and it truncate contents.
FileUtils::getDataFormFile() and FileUtils::getStringFormFile() does not tell different from empty file and io errors.

Advantage of FileUtils::getContents()

Support read into cocos2d::Data std::basic_string and std::vector
It has abstract of the output buffer, custom type of buffer object can be adapted by writing subclass ResizableBuffer.
It returns error code, so that we can know the different form empty file and io error.

minggo · 2016-04-21T09:13:47Z

cocos/platform/CCFileUtils.h

+        typename T,
+        typename std::enable_if<
+            std::is_base_of< ResizableBuffer, ResizableBufferAdapter<T> >::value
+        >::type* = nullptr


I know the effect of enable_if<>, but i don't know the effect of

typename std::enable_if<xx>::type* = nullptr

What does it mean? I can not find the document for it.

http://en.cppreference.com/w/cpp/types/enable_if See Example section #4 usage.

Yep, i have seen it, but don't understand, why can not use

typename std::enable_if<xx>::type

It will cause Substitution Failure if ResizableBufferAdapter<T> is not a subcalss of ResizableBuffer.

I only defined ResizableBufferAdapter< std::vector<> > ResizableBufferAdapter< std::string<> > and ResizableBufferAdapter< Data > inherited form ResizableBuffer. So the template version of getContents() will enabled only for std::vector<> std::string<> and Data.

If user defined his own version ResizableBufferAdapter< UserType >, that type UserType will also suitable for calling template version of getContents().

I know the meaning, but don't understand why need to assign to nullptr. Ok, i will do more research about it.

Thanks.

template < typename T, typename X = std::enable_if< std::is_base_of< ResizableBuffer, ResizableBufferAdapter<T> >::value >::type >

will not compile:

CCFileUtils.h:206:22: error: need 'typename' before 'std::enable_if<std::is_base_of<cocos2d::ResizableBuffer, cocos2d::ResizableBufferAdapter<T> >::value>::type' because 'std::enable_if<std::is_base_of<cocos2d::ResizableBuffer, cocos2d::ResizableBufferAdapter<T> >::value>' is a dependent scope typename X = std::enable_if< ^

Got it, thanks.

But the #5 section of http://en.cppreference.com/w/cpp/types/enable_if can work.

This works:

template < typename T, typename X = typename std::enable_if< std::is_base_of< ResizableBuffer, ResizableBufferAdapter<T> >::value >::type >

#5 section of http://en.cppreference.com/w/cpp/types/enable_if uses enable_if_t (note the tailling _t ) is a C++14 feature.

minggo · 2016-04-21T10:08:41Z

I found only Android platform implements FileError getContents(const std::string& filename, ResizableBuffer* buffer), what will happen if it is invoked in other platforms?

xpol · 2016-04-21T10:13:20Z

There is default: FileError FileUtils::getContents(const std::string& filename, ResizableBuffer* buffer) in CCFileUtils.cpp.

minggo · 2016-04-21T10:17:06Z

Sorry about my careless. I will continue viewing it tomorrow.

xpol · 2016-04-21T10:18:37Z

@minggo
Thank you very much.

minggo · 2016-04-22T02:06:31Z

cocos/platform/CCFileUtils.h

+template<typename T, typename A>
+class ResizableBufferAdapter< std::vector<T, A> > : public ResizableBuffer {
+    typedef std::vector<T, A> BufferType;
+    BufferType* buffer_;


buffer_ -> _buffer to obey the cocos2d-x coding style

minggo · 2016-04-22T02:43:21Z

I run the test case on Mac, it said getContents() failed: error: sbuf != dbuf.

And can we use FileUtils::getContents() to implement FileUtils::getDataFromFile() and FileUtils::getStringFromFile()? Then it will remove duplicated codes.

xpol · 2016-04-22T03:16:01Z

FileUtils::getStringFromFile() is actually bad:

Performance: read whole file, then truncate.
Portable: Text mode is not portable [FileUtils] Should not support text mode. #15456
Memory Usage: use twice memory and copy memory around, 1st whole to the buffer, the copy to a string.
Memory leak: [FileUtils] memory leak when read emty file. #15457

Does the new code of FileUtils::getStringFromFile() needs to implement the truncate and convert CRLF?
For convert CRLF, do I need to convert CRLF only for Windows (that is the current behavior) ?

minggo · 2016-04-22T03:23:10Z

@xpol What i mean is use FileUtils::getContents() internally in FileUtils::getStringFromFile(), for example

 FileUtils::getStringFromFile()
{
    ...
    getContents(...); // use getContents() to get needed result
}

xpol · 2016-04-22T03:28:04Z

Yes.

Does the new code of FileUtils::getStringFromFile() needs to implement the truncate and convert CRLF?
For converting CRLF, do I need to convert CRLF only for Windows (that is the current behavior) ?

minggo · 2016-04-22T03:47:17Z

@xpol I think it is better to keep compatibility and fix bugs in other pull request.

minggo · 2016-04-22T06:49:09Z

cocos/platform/CCFileUtils.h

+public:
+    explicit ResizableBufferAdapter(BufferType* buffer) : _buffer(buffer) {}
+    virtual void resize(size_t size) override {
+        _buffer->resize((size + sizeof(C) - 1) / sizeof(C));


Why the size is (size + sizeof(C) - 1) / sizeof(C)

because that the char type for string and member type for vector may not 8 bit.

But i think std::basic_string<> has already handles it.

std::basic_string<>::size() and std::basic_string<>::resize() is talking about the number of elements (yes the name make users think it's bytes size).

So the size here is not the number of element? Why?

The ResizableBuffer::resize() is about file bytes.

When C is 8 bit. std::basic_string<char>::size() == file bytes
When C is not 8 bit: std::basic_string<wchar_t>::size() != file bytes

What i mean is why size in ResizableBuffer::resize() is bytes. If you want to read wchar_t, then developers should calculate the size of bytes itself?

ResizableBuffer::resize() is not for user calling, FileUtils::getContents() calls it internally:

ResizableBuffer::resize() is called by ``at cocos/platform/CCFileUtils.cpp[line 687](https://github.com/cocos2d/cocos2d-x/pull/15479/files#diff-cb65223cd3edf30671fefad4a1883edfR687) inFileUtils::getContents()` when the file size is determined, and thus, the file size is passed to `ResizableBuffer::resize()`.

ResizableBufferAdapter convert bytes to number of elements need.

Example Usage:

std::string<char32_t> buf; fs->getContents(file, &buf); // resize buf automatically inside getContents() std::cout << "I got " << buf.size() << " of wchar32_t" << std::endl;

Got it, thanks.

methods are: * FileUtils::getDataFromFile() * FileUtils::getStringFromFile() * FileUtils::getFileData() and follow Android methods are now just calls FileUtils' ones. * FileUtilsAndroid::getDataFromFile() * FileUtilsAndroid::getStringFromFile() * FileUtilsAndroid::getFileData()

xpol · 2016-04-22T07:03:37Z

@minggo
Shall I remove follow API from FileUtilsAndroid as they are not necessary now (4c6e4dc):

FileUtilsAndroid::getDataFromFile()
FileUtilsAndroid::getStringFromFile()
FileUtilsAndroid::getFileData()

minggo · 2016-04-25T02:35:56Z

cocos/platform/CCFileUtils.cpp

+{
+    // TODO: move this function to StringUtils.
+    // http://stackoverflow.com/a/7724536/87021
+    static inline std::string replaceAll(


I think static is not need here.

@xpol I think static is not needed when it is in an anonymous namespace.

…tils-get-contents

…os2d-x into file-utils-get-contents # Conflicts: # cocos/platform/CCFileUtils.cpp # tests/cpp-tests/Classes/FileUtilsTest/FileUtilsTest.cpp

xpol · 2016-04-25T07:30:16Z

@minggo
Have you got any idea about the Linux build error:

In file included from /linux_slave/workspace/ccs-pr/node/linux/cocos/platform/linux/CCDevice-linux.cpp:30:0:
/linux_slave/workspace/ccs-pr/node/linux/cocos/platform/CCFileUtils.h:164:16: error: anonymous scoped enum is not allowed
     enum class Status
                ^
In file included from /linux_slave/workspace/ccs-pr/node/linux/cocos/platform/linux/CCDevice-linux.cpp:39:0:
/linux_slave/workspace/ccs-pr/node/linux/cocos/platform/CCFileUtils.h:164:5: warning: elaborated-type-specifier for a scoped enum must not use the ‘class’ keyword
     enum class Status
     ^
In file included from /linux_slave/workspace/ccs-pr/node/linux/cocos/platform/linux/CCDevice-linux.cpp:30:0:
/linux_slave/workspace/ccs-pr/node/linux/cocos/platform/CCFileUtils.h:164:16: error: expected identifier before ‘int’
     enum class Status
                ^
In file included from /linux_slave/workspace/ccs-pr/node/linux/cocos/platform/linux/CCDevice-linux.cpp:39:0:
/linux_slave/workspace/ccs-pr/node/linux/cocos/platform/CCFileUtils.h:165:5: error: expected unqualified-id before ‘{’ token
     {
     ^
cocos/CMakeFiles/cocos2dInternal.dir/build.make:2607: recipe for target 'cocos/CMakeFiles/cocos2dInternal.dir/platform/linux/CCDevice-linux.cpp.o' failed

minggo · 2016-04-25T07:59:45Z

Remove the last ,?

Status must defined in some header, so move FileUtils.h up.

minggo · 2016-04-25T08:51:47Z

About the linux error, i think it is because some header files define a macro named Status, such as

#define Status

xpol · 2016-04-25T09:02:04Z

Yes, I have moved the include of CCFileUtils.h up in cocos\platform\linux\CCDevice-linux.cpp

xpol · 2016-04-26T02:25:23Z

ping

minggo · 2016-04-26T02:55:21Z

cocos/platform/CCFileUtils.cpp

+    bool textmode = strchr(mode, 't') != NULL;
+    if (textmode)
+        textFormCRLF(s);
+#endif


I think these codes should be applied to WIN32 too. Sorry for my previous answer about it. And NULL -> nullptr

According to comments above in our current code base :

For FileUtils::getStringFromFile() : do not convert CRLF to LF on all platform including Win32 and WinRT.

For FileUtils::getFileData() : Only WinRT do CRLF to LF conversion ( mode arg is not used on Win32 ). And I guess this is a bug.

Did you mean in this PR, we shall do support CRLF to LF on Win32 and WinRT for both FileUtils::getStringFromFile() and FileUtils::getFileData()?

IMHO, we shold remove all support of CRLF to LF behavior as:

it's not portable (and cocos2d-x is a cross platform engine).

currently only FileUtils::getFileData() on WinRT support CRLF to LF.

all other case (non WinRT or not FileUtils::getFileData()), the current behavior is not convert CRLF to LF.

Sorry, i misunderstood of the logic of textFormCRLF. Yep, i agree with you, we should not convert CRLF to LF. So i think we can remove these codes.

minggo · 2016-04-26T02:56:14Z

@xpol I added comments above. I think i can merge it when it is resolved. Thanks.

xpol · 2016-04-26T03:25:01Z

@minggo

I updated the line note, I just want to make it sure.

Would please inspect the the links in above line note link to current v3 code if you needs detailed information.

minggo · 2016-04-26T03:34:28Z

@xpol I think we can remove the codes

+#if CC_TARGET_PLATFORM == CC_PLATFORM_WINRT
+    // TODO: only winrt support text mode. it is possibly a bug. if true, remove the follow code for text mode.
+    bool textmode = strchr(mode, 't') != NULL;
+    if (textmode)
+        textFormCRLF(s);
+#endif

And static is not needed if a function is in an anonymous namespace.

… text mode).

xpol · 2016-04-26T03:44:44Z

OK then all platform has the same behavior: no text mode support for file reading.

BTW, the static and NULL issues is gone as they are in the CRLF to LF support code.

minggo · 2016-04-26T03:47:08Z

Thanks. I think it is ok to do it like this. And let's fix it if we met issues in future.

xpol · 2016-04-26T03:50:12Z

Glad I can help.

* Add FileUtils::getContents(). * skip FileUtils::getContents() in binding generator config. * use FileUtils::getContents in CCDataReaderHelper. * obey the cocos2d-x coding style. * Explicit constructor. * More docs. * More tests. * Move FileError to FileUtils::Error. * Fixes wrong buffer size for reading into string and vector. * Update tests. * Add note on padding for output buffers. * FileUtils: implements old methods by using `getContents()`. methods are: * FileUtils::getDataFromFile() * FileUtils::getStringFromFile() * FileUtils::getFileData() and follow Android methods are now just calls FileUtils' ones. * FileUtilsAndroid::getDataFromFile() * FileUtilsAndroid::getStringFromFile() * FileUtilsAndroid::getFileData() * Fixes build error. * FileUtils::getFileData: Return the size of data. * Remove old methods form FileUtilsAndroid they are now done in FileUtils. * Fixes for win32 code. * Fixes build error in test and add more test. * Better error message. * Make template type name more readable. * Update comments. * Move internal functions to anonymous namespace. * Refactor FileUtils test. * Fix warning about compare signed and unsigned. * Win32 and WinRT does not use text mode. That is we don't need simulate convert CRLF to LF. * Fixes for Win32 and WinRT. * Update for Win32 and WinRT. * Win32: return FileUtils:Error::TooLarge when file is large than 2^32-1. * Win32: remove checkFileName() which has no effect at all. * WinRT: add FileUtilsWinRT::getContents() using ::CreateFile2. * WinRT: add override keyword for FileUtilsWinRT::getFileSize(). * Update for coding styles. * More error strings. * check read and malloc return codes. * rename FileUtils::Error to FileUtils::Status. * Fixes for WinRT, use GetFileInformationByHandleEx to get file size. * Fixes build error for winrt and cleanup FileUtils::Status. * Try to fix the build error on Linux. Status must defined in some header, so move FileUtils.h up. * Remove support of text mode on WinRT (it is the last platform support text mode).

xpol added 3 commits April 21, 2016 13:06

Add FileUtils::getContents().

c689fa3

skip FileUtils::getContents() in binding generator config.

a61b693

use FileUtils::getContents in CCDataReaderHelper.

d17a636

xpol force-pushed the file-utils-get-contents branch from c374605 to d17a636 Compare April 21, 2016 05:55

minggo reviewed Apr 21, 2016
View reviewed changes

minggo self-assigned this Apr 21, 2016

minggo reviewed Apr 22, 2016
View reviewed changes

obey the cocos2d-x coding style.

4721e97

xpol added 3 commits April 22, 2016 11:02

Explicit constructor.

d5b31fd

More docs.

e78c6f9

More tests.

1e44fca

Move FileError to FileUtils::Error.

ce2ab6f

xpol added 3 commits April 22, 2016 13:28

Fixes wrong buffer size for reading into string and vector.

36fe65d

Update tests.

c1555ae

Add note on padding for output buffers.

ce9c7c4

minggo reviewed Apr 22, 2016
View reviewed changes

Fixes build error.

0f13b53

minggo reviewed Apr 25, 2016
View reviewed changes

xpol added 7 commits April 25, 2016 13:51

Update for coding styles.

bfd0f8f

Merge branch 'v3' of https://github.com/cocos2d/cocos2d-x into file-u…

f3ac905

…tils-get-contents

Merge branch 'file-utils-get-contents' of https://github.com/xpol/coc…

71267d8

…os2d-x into file-utils-get-contents # Conflicts: # cocos/platform/CCFileUtils.cpp # tests/cpp-tests/Classes/FileUtilsTest/FileUtilsTest.cpp

More error strings.

41a7a41

check read and malloc return codes.

f2993de

rename FileUtils::Error to FileUtils::Status.

d454593

Fixes for WinRT, use GetFileInformationByHandleEx to get file size.

8e53ac9

xpol added 2 commits April 25, 2016 16:11

Fixes build error for winrt and cleanup FileUtils::Status.

9edf2f2

Try to fix the build error on Linux.

0bc05df

Status must defined in some header, so move FileUtils.h up.

minggo reviewed Apr 26, 2016
View reviewed changes

Remove support of text mode on WinRT (it is the last platform support…

653090b

… text mode).

minggo merged commit 43a94c0 into cocos2d:v3 Apr 26, 2016

minggo added module:core labels Apr 26, 2016

minggo added this to the next milestone Apr 26, 2016

xpol deleted the file-utils-get-contents branch April 26, 2016 05:59

Add FileUtils::getContents(). #15479

Add FileUtils::getContents(). #15479

Conversation

xpol commented Apr 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpol Apr 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpol Apr 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpol Apr 21, 2016 • edited

Choose a reason for hiding this comment

minggo commented Apr 21, 2016

xpol commented Apr 21, 2016 • edited

minggo commented Apr 21, 2016

xpol commented Apr 21, 2016

Choose a reason for hiding this comment

minggo commented Apr 22, 2016

xpol commented Apr 22, 2016 • edited

minggo commented Apr 22, 2016

xpol commented Apr 22, 2016 • edited

minggo commented Apr 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpol Apr 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpol commented Apr 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpol commented Apr 25, 2016

minggo commented Apr 25, 2016

minggo commented Apr 25, 2016

xpol commented Apr 25, 2016

xpol commented Apr 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minggo commented Apr 26, 2016

xpol commented Apr 26, 2016 • edited

minggo commented Apr 26, 2016

xpol commented Apr 26, 2016 • edited

minggo commented Apr 26, 2016

xpol commented Apr 26, 2016

xpol Apr 21, 2016 •

edited

xpol Apr 21, 2016 •

edited

xpol Apr 21, 2016 •

edited

xpol commented Apr 21, 2016 •

edited

xpol commented Apr 22, 2016 •

edited

xpol commented Apr 22, 2016 •

edited

xpol Apr 22, 2016 •

edited

xpol commented Apr 22, 2016 •

edited

xpol commented Apr 26, 2016 •

edited

xpol commented Apr 26, 2016 •

edited