[Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

Zachary Turner via lldb-commits lldb-commits at lists.llvm.org
Mon Mar 21 11:02:33 PDT 2016


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





More information about the lldb-commits mailing list