Page MenuHomePhabricator

[PATCH] More UTF string conversion wrappers
ClosedPublic

Authored by cameron314 on Feb 23 2016, 12:00 PM.

Details

Summary

(And a missing string conversion for getenv on Win32, too.)

These new string conversion wrappers convert between std::string (of UTF-8 bytes) and std::wstring, which is particularly useful for Win32 interop.

The motivation behind this is to support a patch I'm preparing for LLDB (the first version of which is at D17107) that makes heavy use of these helper functions for better Unicode support on Windows.

Diff Detail

Repository
rL LLVM

Event Timeline

cameron314 retitled this revision from to More UTF string conversion wrappers.
cameron314 updated this object.
cameron314 added a reviewer: zturner.
cameron314 added a subscriber: llvm-commits.
cameron314 retitled this revision from More UTF string conversion wrappers to [PATCH] More UTF string conversion wrappers.Feb 23 2016, 12:44 PM

This patch is applicable as is to rL262844 (the tip a short while ago).

Sorry, I didn't see this one come through before.

Adding a few other LLVM windows people, as they are probably more qualified to review something on the LLVM side than I am.

aaron.ballman added inline comments.Mar 8 2016, 10:52 AM
llvm/trunk/include/llvm/Support/ConvertUTF.h
204

& 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.

216

Inconsistent capitalization of the function name.

llvm/trunk/lib/Support/CommandLine.cpp
793

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.

799

Same for this failure.

llvm/trunk/lib/Support/ConvertUTFWrapper.cpp
141

This function overload does not appear to be used, is it required?

cameron314 added inline comments.Mar 8 2016, 1:33 PM
llvm/trunk/include/llvm/Support/ConvertUTF.h
204

Oops, missed these, will fix!

216

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 ;-)

llvm/trunk/lib/Support/CommandLine.cpp
793

Hmm, good point. I'll add asserts.

llvm/trunk/lib/Support/ConvertUTFWrapper.cpp
141

Hmm, you're right. It was used by an earlier version of my patch for LLDB, but no longer. I'll remove it.

cameron314 updated this revision to Diff 50073.Mar 8 2016, 2:39 PM
cameron314 edited edge metadata.
aaron.ballman edited edge metadata.Mar 8 2016, 3:11 PM

Some small nits, but basically looking good.

llvm/trunk/lib/Support/CommandLine.cpp
793

Can you turn them into assert(false && "Message about what went wrong");?

llvm/trunk/lib/Support/ConvertUTFWrapper.cpp
191

You should run clang-format over this as well, the formatting isn't correct (here and elsewhere in the file).

244

Use llvm_unreachable?

cameron314 added inline comments.Mar 8 2016, 3:28 PM
llvm/trunk/lib/Support/CommandLine.cpp
793

Will do.

llvm/trunk/lib/Support/ConvertUTFWrapper.cpp
191

Will do. I was hoping to avoid this since clang-format changes a lot of code that was already present as well. I'll extract just the parts that apply to my patch, I suppose.

244

Aha, didn't know it existed :-)

aaron.ballman added inline comments.Mar 8 2016, 3:30 PM
llvm/trunk/lib/Support/ConvertUTFWrapper.cpp
191

You can run clang-format over the patch file directly, too. http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

cameron314 updated this revision to Diff 50084.Mar 8 2016, 3:34 PM
cameron314 edited edge metadata.
aaron.ballman accepted this revision.Mar 8 2016, 3:37 PM
aaron.ballman edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Mar 8 2016, 3:37 PM

Great, thanks for the review!
I don't have commit access, can someone commit this on my behalf?

This revision was automatically updated to reflect the committed changes.