[PATCH] D17549: [PATCH] More UTF string conversion wrappers

[Cameron via llvm-commits]

cameron314 added inline comments.

================
Comment at: llvm/trunk/include/llvm/Support/ConvertUTF.h:204
@@ +203,3 @@
+*/
+bool ConvertUTF8toWide(llvm::StringRef Source, std::wstring& Result);
+
----------------
aaron.ballman wrote:
> & and * bind to the identifier instead of the type in our coding conventions (here and elsewhere). Run the patch through clang-format and it should fix all of those up automatically, as well as other formatting issues in the patch.
Oops, missed these, will fix!

================
Comment at: llvm/trunk/include/llvm/Support/ConvertUTF.h:216
@@ +215,3 @@
+*/
+bool convertWideToUTF8(std::wstring const& Source, std::string& Result);
+
----------------
aaron.ballman wrote:
> Inconsistent capitalization of the function name.
It was already inconsistent. I had to keep `ConvertUTF8toWide` (with the capital 'C' and lower 't') because it already existed and I was adding overloads.

However, all the other functions in this file (and LLVM generally, I believe) follow the camelcase-starting-with-lower style for global functions. So I tried to pick the lesser of two evils ;-)

================
Comment at: llvm/trunk/lib/Support/CommandLine.cpp:793
@@ +792,3 @@
+  if (!llvm::ConvertUTF8toWide(envVar, wenvVar))
+    return;
+  const wchar_t *wenvValue = _wgetenv(wenvVar.c_str());
----------------
aaron.ballman wrote:
> I feel like maybe we shouldn't silently eat this sort of failure, but should either report a fatal error or assert. I don't feel strongly about it, however.
Hmm, good point. I'll add asserts.

================
Comment at: llvm/trunk/lib/Support/ConvertUTFWrapper.cpp:140
@@ +139,3 @@
+
+bool convertUTF16ToUTF8String(const UTF16 *Src, std::string &Out)
+{
----------------
aaron.ballman wrote:
> This function overload does not appear to be used, is it required?
Hmm, you're right. It was used by an earlier version of my patch for LLDB, but no longer. I'll remove it.


http://reviews.llvm.org/D17549