All done (except for the static_cast, since that line also converts signedness).<div><br></div><div>Any comments on the new file name / location from anyone?</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jun 27, 2012 at 7:06 AM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Jun 25, 2012 at 4:54 AM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>

> Hi,<br>
><br>
> the attached patch moves the duplicated code in CGExpr.cpp and<br>
> LiteralSupport.cpp into a new file Basic/ConvertUTFWrapper.h/cpp.<br>
> Please let me know what you think.<br>
><br>
> Nico<br>
<br>
</div></div>Looks reasonable as far as I can tell. Just some nits:<br>
<br>
> +#ifndef LLVM_CLANG_BASIC_CONVERT_UTF_WRAPPER_H<br>
> +#define LLVM_CLANG_BASIC_CONVERT_UTF_WRAPPER_H<br>
> +<br>
> +#include "llvm/ADT/StringRef.h"<br>
> +<br>
> +namespace clang {<br>
> +<br>
> +/// Convert an UTF8 StringRef to UTF8, UTF16, or UTF32 depending on<br>
> +/// WideCharWidth. On success, ResultPtr will point one after the end of the<br>
> +/// copied string.<br>
> +/// \returns true on success.<br>
> +bool ConvertUTF8toWide(unsigned WideCharWidth, llvm::StringRef Source,<br>
> +                       char*& ResultPtr);<br>
<br>
Should the *& be on ResultPtr here? (and in the .cpp file)<br>
Should the comment say that it's copying into ResultPtr?<br>
<br>
> Index: lib/Basic/ConvertUTFWrapper.cpp<br>
> ===================================================================<br>
> --- lib/Basic/ConvertUTFWrapper.cpp   (revision 0)<br>
> +++ lib/Basic/ConvertUTFWrapper.cpp   (revision 0)<br>
> @@ -0,0 +1,55 @@<br>
> +//===-- ConvertUTFWrapper.cpp - Wrap ConvertUTF.h with clang data types -----===<br>
> +//<br>
> +//                     The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +#include "clang/Basic/ConvertUTFWrapper.h"<br>
> +#include "clang/Basic/ConvertUTF.h"<br>
> +#include "clang/Basic/LLVM.h"<br>
> +<br>
> +namespace clang {<br>
> +<br>
> +bool ConvertUTF8toWide(unsigned WideCharWidth, llvm::StringRef Source,<br>
> +                       char*& ResultPtr) {<br>
> +  assert(WideCharWidth==1 || WideCharWidth==2 || WideCharWidth==4);<br>
> +  ConversionResult result = conversionOK;<br>
> +  // Copy the character span over.<br>
> +  if (WideCharWidth == 1) {<br>
> +    if (!isLegalUTF8String(reinterpret_cast<const UTF8*>(&*Source.begin()),<br>
> +                           reinterpret_cast<const UTF8*>(&*Source.end())))<br>
<br>
The &* isn't really necessary for Source.begin() / end(). Do we<br>
normally do this for StringRefs? (I see it wasn't there in<br>
lib/Lex/LiteralSupport.cpp)<br>
<br>
> +      result = sourceIllegal;<br>
> +    memcpy(ResultPtr, Source.data(), Source.size());<br>
> +    ResultPtr += Source.size();<br>
> +  } else if (WideCharWidth == 2) {<br>
> +    UTF8 const *sourceStart = (UTF8 const *)Source.data();<br>
<br>
This is equivalent to "const UTF8 *sourceStart", right? At least I<br>
find that easier to read. And maybe static_cast it?<br>
<br>
> +    // FIXME: Make the type of the result buffer correct instead of<br>
> +    // using reinterpret_cast.<br>
> +    UTF16 *targetStart = reinterpret_cast<UTF16*>(ResultPtr);<br>
> +    ConversionFlags flags = strictConversion;<br>
> +    result = ConvertUTF8toUTF16(<br>
> +      &sourceStart,sourceStart + Source.size(),<br>
<br>
Nit: space between &targetStart and targetStart?<br>
<br>
> +        &targetStart,targetStart + 2*Source.size(),flags);<br>
<br>
And some more spaces here too..<br>
<br>
> +    if (result==conversionOK)<br>
<br>
And here :)<br>
<br>
> +      ResultPtr = reinterpret_cast<char*>(targetStart);<br>
> +  } else if (WideCharWidth == 4) {<br>
> +    UTF8 const *sourceStart = (UTF8 const *)Source.data();<br>
<br>
Ditto about "const UTF8* sourceStart" etc.<br>
<br>
> +    // FIXME: Make the type of the result buffer correct instead of<br>
> +    // using reinterpret_cast.<br>
> +    UTF32 *targetStart = reinterpret_cast<UTF32*>(ResultPtr);<br>
> +    ConversionFlags flags = strictConversion;<br>
> +    result = ConvertUTF8toUTF32(<br>
<br>
> +        &sourceStart,sourceStart + Source.size(),<br>
> +        &targetStart,targetStart + 4*Source.size(),flags);<br>
> +    if (result==conversionOK)<br>
<br>
Ditto about spaces.<br>
<br>
Thanks,<br>
Hans<br>
</blockquote></div><br></div>