On Friday, October 4, 2013, Alp Toker  wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi David,<br>
<br>
The switch to wide functions makes sense. Maybe we can make the changes<br>
a little lighter..<br>
<br>
+  SmallVector<wchar_t, 128> ProgramUtf16;<br>
+  if (error_code ec = windows::UTF8ToUTF16(Program, ProgramUtf16)) {<br>
+    SetLastError(ec.value());<br>
+    MakeErrMsg(ErrMsg,<br>
+               std::string("Unable to convert application name to<br>
UTF-16"));<br>
+    return false;<br>
+  }<br>
+<br>
+  SmallVector<wchar_t, 128> CommandUtf16;<br>
+  if (error_code ec = windows::UTF8ToUTF16(command.get(), CommandUtf16)) {<br>
+    SetLastError(ec.value());<br>
+    MakeErrMsg(ErrMsg,<br>
+               std::string("Unable to convert command-line to UTF-16"));<br>
+    return false;<br>
+  }<br>
<br>
There are quite a few of these.<br>
<br>
LLVM doesn't generally report UTF8ToUTF16() failure beyond propagating<br>
the error. Is conversion failure likely and user-actionable enough to<br>
warrant individually tailored messages? Maybe a general "UTF-16<br>
conversion failure" message lifted into an inline function would suffice?</blockquote><div><br></div><div>It's unlikely to ever trigger but I figured that if it ever did, I'd like to know why.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, UTF8ToUTF16() seems to already exit with last error set. Did you<br>
find a need to explicitly SetLastError() after the call?</blockquote><div><br></div><div>I prefer doing this explicitly because I don't want to rely on the implementation details of the function. It returns an error_code and not a bool, if it were designed to purposely set the last error, that would be a part of it's specified interface.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
So, if each of these conversions can be reduced to one or two LoC I<br>
think it'll make your patch more readable, and also make it more likely<br>
for other code in LLVM to follow your example and switch to the wide API.<br>
<br>
Maybe something like this, and then you could chain the conversions as<br>
well..<br>
<br>
SmallVector<wchar_t, 128> ProgramUtf16, CommandUtf16;<br>
if (!ConvertToUTF16(Program, ProgramUtf16, ErrMsg) ||<br>
    !ConvertToUTF16(Command, CommandUtf16, ErrMsg))<br>
  return false;<br>
<br>
Alp.<br>
<br>
<br>
<br>
On 04/10/2013 13:14, David Majnemer wrote:<br>
> Hi aaron.ballman, Bigcheese, rnk, ruiu,<br>
><br>
> The MSVCRT deliberately sends main() code-page specific characters.<br>
> This isn't too useful to LLVM as we end up converting the arguments to<br>
> UTF-16 and subsequently attempt to use the result as, for example, a<br>
> file name.  Instead, we need to have the ability to access the Unicode<br>
> command line and transform it to UTF-8.<br>
><br>
> This has the distinct advantage over using the MSVC-specific wmain()<br>
> function as our entry point because:<br>
>  - It doesn't work on cygwin.<br>
>  - It only work on MinGW with caveats and only then on certain versions.<br>
>  - We get to keep our entry point as main(). :)<br>
><br>
> N.B.  This patch includes fixes to other parts of lib/Support/Windows<br>
> s.t. we would be able to take advantage of getting the Unicode paths.<br>
> E.G.  clang spawning clang -cc1 would want to give it Unicode arguments.<br>
><br>
> <a href="http://llvm-reviews.chandlerc.com/D1834" target="_blank">http://llvm-reviews.chandlerc.com/D1834</a><br>
><br>
> Files:<br>
>   autoconf/<a href="http://configure.ac" target="_blank">configure.ac</a><br>
>   cmake/config-ix.cmake<br>
>   cmake/modules/LLVM-Config.cmake<br>
>   cmake/modules/TableGen.cmake<br>
>   configure<br>
>   include/llvm/Config/config.h.cmake<br>
>   include/llvm/Config/<a href="http://config.h.in" target="_blank">config.h.in</a><br>
>   include/llvm/Support/Process.h<br>
>   lib/Support/Unix/Process.inc<br>
>   lib/Support/Windows/DynamicLibrary.inc<br>
>   lib/Support/Windows/Path.inc<br>
>   lib/Support/Windows/Process.inc<br>
>   lib/Support/Windows/Program.inc<br>
>   lib/Support/Windows/Signals.inc<br>
>   lib/Support/Windows/Windows.h<br>
>   projects/sample/autoconf/<a href="http://configure.ac" target="_blank">configure.ac</a><br>
>   projects/sample/configure<br>
>   utils/FileCheck/CMakeLists.txt<br>
>   utils/FileUpdate/CMakeLists.txt<br>
>   utils/not/CMakeLists.txt<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="javascript:;" onclick="_e(event, 'cvml', 'llvm-commits@cs.uiuc.edu')">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
--<br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</blockquote>