Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite portions of EbrFile handling to use UTF16 #1891

Merged
merged 8 commits into from
Feb 2, 2017
4 changes: 2 additions & 2 deletions Frameworks/Foundation/NSPathUtilities.mm
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@
// Helper that gets the path for a folder dirName under the current app's AppData/Local... directory,
// creating the folder if necessary
NSString* _getCreateAppDataLocalDir(const char* dirName) {
auto ret = [NSString stringWithFormat:@"%hs/%hs", EbrGetWritableFolder(), dirName];
auto ret = [NSString stringWithFormat:@"%S/%hs", EbrGetWritableFolder(), dirName];
_mkdir([ret cStringUsingEncoding:NSUTF8StringEncoding]);
return ret;
}

// Override for when a higher-level directory needs to be created first (eg: Foo1/Foo2/)
NSString* _getCreateAppDataLocalDir(const char* dirName1, const char* dirName2) {
_getCreateAppDataLocalDir(dirName1);
auto ret = [NSString stringWithFormat:@"%hs/%hs/%hs", EbrGetWritableFolder(), dirName1, dirName2];
auto ret = [NSString stringWithFormat:@"%S/%hs/%hs", EbrGetWritableFolder(), dirName1, dirName2];
_mkdir([ret cStringUsingEncoding:NSUTF8StringEncoding]);
return ret;
}
Expand Down
3 changes: 2 additions & 1 deletion Frameworks/Starboard/AssetFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,9 @@ bool EbrFSDirReader::readNext(EbrDir* curDir, EbrDirEnt* ent) {
EbrDir* EbrOpenDir(const char* path) {
CPathMapper map(path);

if (!map)
if (!map) {
return NULL;
}

EbrDirReader* fsReader = EbrFSDirReader::open(map.MappedPath());
if (!fsReader) {
Expand Down
12 changes: 8 additions & 4 deletions Frameworks/Starboard/EbrFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,17 @@ bool EbrUnlink(const char* path) {
}

#define mkdir _mkdir
char g_WritableFolder[2048] = ".";
std::wstring g_WritableFolder(L".");

void EbrSetWritableFolder(const char* folder) {
strcpy_s(g_WritableFolder, folder);
void IwSetWritableFolder(const wchar_t* folder) {
g_WritableFolder = folder;
}

const char* EbrGetWritableFolder() {
const wchar_t* IwGetWritableFolder() {
return g_WritableFolder.c_str();
}

const std::wstring& _IwGetWritableFolder() {
return g_WritableFolder;
}

Expand Down
19 changes: 9 additions & 10 deletions Frameworks/Starboard/PathMapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,11 @@

#include <algorithm>
#include <list>
#include <regex>

#include "Platform/EbrPlatform.h"
#include "PathMapper.h"

// utility function to tokenize string using delmiters
// utility function to tokenize string using delimiters
// d:\src/winobjc ==> d:, src, winobjc
// /src/winobjc ==> "", src, winobjc

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the presence of a "" always mean it's an absolute unix path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It just means that it was an absolute path.

// / ==> ""
Expand Down Expand Up @@ -59,9 +58,9 @@ static void _EscapeIllegalPathCharacters(std::wstring& str) {
}

// normalize relative path
// 1 remove any "." or "" components
// 2 from reverse, remove any ".." and preceeding component
// if components becomes empty, return "."
// 1 remove any "." or "" components
// 2 remove any ".." and preceeding component
// 3 if components becomes empty, return "."

static void _NormalizeRelativePathComponents(std::list<std::wstring>& components) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above :).

for (auto it = components.begin(); it != components.end();) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put ++it in here rather than in last else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. continue also goes to ++.

Expand All @@ -76,6 +75,8 @@ static void _NormalizeRelativePathComponents(std::list<std::wstring>& components
break;
}
// we have to do this in case the last component is ..
// erase the previous component first because erase returns the next item
// which would be "..".
it = components.erase(--it);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we concerned about reverse-iterating a std::list? I thought they were singly-linked, making -- a taxing operator, but perhaps they're not!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can be double linked I thought, but given how we're using it might make more sense to just use a std::vector. FWIU it's generally better to take a hit every once in a while for resizing if we need to than having to jump around when iterating.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also don't like this expression it = something(--it), hard to figure out exactly what's going on

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::list is bidirectional. Not changing this to vector as sometime in the future when we do properly support > MAX_PATH, there will be several elements here. And leading '.' is pretty common. Since the list is all constructed at the same time, locality of reference should not be a problem.

it = components.erase(it);
} else {
Expand All @@ -98,15 +99,13 @@ std::wstring _MapPathRoot(const std::wstring& root) {
return std::wstring(L".\\home");
}

std::wregex drive(L"[a-zA-Z]:");

if (std::regex_match(root, drive)) {
if (root.length() == 2 && iswalpha(root[0]) && root[1] == L':') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return root;
}

for (int i = 0; i < _countof(c_specialFolders); ++i) {
for (auto root : c_specialFolders) {
if (_wcsicmp(root.c_str(), c_specialFolders[i].c_str()) == 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since both of these are std::wstrings, it'll be better to use == comparison (it can optimize for length differences better than _wcsicmp)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, i need it to be case insensitive and == isn't.

return Strings::NarrowToWide<std::wstring>(EbrGetWritableFolder()) + std::wstring(L"\\") + c_specialFolders[i];
return IwGetWritableFolder() + std::wstring(L"\\") + c_specialFolders[i];
}
}

Expand Down
7 changes: 3 additions & 4 deletions Frameworks/UIKit/StarboardXaml/ApplicationCompositor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,11 @@ void InitializeApp() {
initialized = true;

// Set our writable and temp folders
char writableFolder[2048];
size_t outLen;
auto pathData = Windows::Storage::ApplicationData::Current->LocalFolder->Path;
wcstombs_s(&outLen, writableFolder, pathData->Data(), sizeof(writableFolder) - 1);
EbrSetWritableFolder(writableFolder);
IwSetWritableFolder(pathData->Data());

char writableFolder[2048];
size_t outLen;
auto tempPathData = Windows::Storage::ApplicationData::Current->TemporaryFolder->Path;
wcstombs_s(&outLen, writableFolder, tempPathData->Data(), sizeof(writableFolder) - 1);
SetTemporaryFolder(writableFolder);
Expand Down
4 changes: 2 additions & 2 deletions Frameworks/include/Platform/EbrPlatform.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ SB_EXPORT int EbrGetTimeOfDay(struct EbrTimeval* curtime);
SB_EXPORT double EbrGetMediaTime();
SB_EXPORT int EbrGetWantedOrientation();

SB_EXPORT const char* EbrGetWritableFolder();
SB_EXPORT void EbrSetWritableFolder(const char* folder);
SB_EXPORT const wchar_t* IwGetWritableFolder();
SB_EXPORT void IwSetWritableFolder(const wchar_t* folder);

SB_EXPORT void EbrBlockIfBackground();

Expand Down
5 changes: 3 additions & 2 deletions build/Starboard/dll/Starboard.def
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,6 @@ LIBRARY Starboard
EbrGetTimeOfDay
EbrGetMediaTime
EbrGetWantedOrientation
EbrGetWritableFolder
EbrSetWritableFolder
EbrBlockIfBackground
EbrEventInit
EbrEventSignal
Expand All @@ -175,5 +173,8 @@ LIBRARY Starboard
EbrEventTimedMultipleWait
EbrEventDestroy

IwGetWritableFolder
IwSetWritableFolder

; REMOVE / CLEANUP C++ EXPORTS
?format@string@woc@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@PBDZZ
8 changes: 6 additions & 2 deletions tests/unittests/Starboard/PathMapperTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@
#include <TestFramework.h>
#include "pathmapper.h"

extern "C" void EbrSetWritableFolder(const char*);
extern "C" void IwSetWritableFolder(const wchar_t*);
extern "C" const wchar_t* IwGetWritableFolder();

TEST(PathMapper, pathMapper) {
EbrSetWritableFolder("d:\\temp");
std::wstring writableFolder = EbrGetWritableFolder();
IwSetWritableFolder(L"d:\\temp");

CPathMapper mapper1("c:/users/winobjc-bot");
EXPECT_EQ_MSG(0, wcscmp(mapper1, L"c:\\users\\winobjc-bot"), "%S", (const wchar_t*)mapper1);
Expand Down Expand Up @@ -69,6 +71,8 @@

CPathMapper mapper16("/AppSupport/.\\?/");
EXPECT_EQ_MSG(0, wcscmp(mapper16, L"d:\\temp\\AppSupport\\+"), "%S", (const wchar_t*)mapper16);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for these guys, prefer EXPECT_STREQ(expected, actual); gtest/taef will properly log the inputs and use the wchar_t overload of StrHelperCmpEQ

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(it means we don't have to rely on the logging infrastructure in the _MSG macro variants, and gtest can print expected/actual next to eachother)


IwSetWritableFolder(writableFolder.c_str());
}

TEST(PathMapper, RelativePathTests) {
Expand Down