zturner added a comment.
Patch doesn't appear to be clang-formatted. All patches need to be run through
clang-format before submitting.
I think this is my last set of comments. If there's any you disagree with let
me know. If you agree with everything, I can probably just make the changes on
my end since they are mostly mechanical, and then submit.
================
Comment at: cmake/modules/LLDBConfig.cmake:250
@@ +249,3 @@
+if (CMAKE_SYSTEM_NAME MATCHES "Windows")
+ add_definitions( /D _UNICODE /D UNICODE )
+endif()
----------------
This is using a tab, but it should be 2 spaces.
================
Comment at: source/Core/ConnectionSharedMemory.cpp:147
@@ +146,3 @@
+ INVALID_HANDLE_VALUE,
+ NULL,
+ PAGE_READWRITE,
----------------
`nullptr` instead of `NULL`
================
Comment at: source/Host/common/FileSpec.cpp:94-115
@@ -93,2 +93,24 @@
+ {
+#ifdef _WIN32
+ std::wstring wresolved_path;
+ if (!llvm::ConvertUTF8toWide(resolved_path, wresolved_path))
+ return false;
+ int stat_result;
+#ifdef _USE_32BIT_TIME_T
+ struct _stat32 file_stats;
+ stat_result = ::_wstat32(wresolved_path.c_str(), &file_stats);
+#else
+ struct _stat64i32 file_stats;
+ stat_result = ::_wstat64i32(wresolved_path.c_str(), &file_stats);
+#endif
+ if (stat_result == 0)
+ {
+ static_assert(sizeof(struct stat) == sizeof(file_stats), "stat and
_stat32/_stat64i32 must have the same layout");
+ *stats_ptr = *reinterpret_cast<struct stat *>(&file_stats);
+ }
+ return stat_result == 0;
+#else
return ::stat (resolved_path, stats_ptr) == 0;
+#endif
+ }
return false;
----------------
Here we are doing some stuff with `_USE_32BIT_TIME_T`, but we reproduce almost
this exact same logic in `File::GetPermissions()` except without the special
case for `_USE_32BIT_TIME_T`. I think we should introduce a function called
`FileSystem::Stat()` in very much the same way that you've done
`FileSystem::Fopen()`. And then we put the implementation there so that we can
have it in one place and make sure it's always going to be consistent and
correct.
================
Comment at: tools/lldb-mi/MIUtilFileStd.cpp:82-95
@@ -79,9 +81,16 @@
#if !defined(_MSC_VER)
// Open with 'write' and 'binary' mode
m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
#else
// Open a file with exclusive write and shared read permissions
- m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+ std::wstring path;
+ if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+ m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+ else
+ {
+ errno = EINVAL;
+ m_pFileHandle = nullptr;
+ }
#endif // !defined( _MSC_VER )
----------------
Should we be using `FileSystem::Fopen()` here?
================
Comment at: tools/lldb-mi/MIUtilFileStd.cpp:234-241
@@ -224,3 +233,10 @@
FILE *pTmp = nullptr;
+#ifdef _WIN32
+ std::wstring path;
+ if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+ return false;
+ pTmp = ::_wfopen(path.c_str(), L"wb");
+#else
pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
if (pTmp != nullptr)
----------------
Same here. `FileSystem::Fopen()`
http://reviews.llvm.org/D17107
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
