[PATCH] D46238: [llvm-rc] Add rudimentary support for codepages

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: tools/llvm-rc/ResourceFileWriter.h:29
+enum CodePage {
+  CP_ACP = 0,         // The current used codepage. Since there's no such
+                      // notion in LLVM what codepage it actually means,
----------------
amccarth wrote:
> Are you concerned about the name clash with the macro defined in the Windows headers?  It seems like code that wants to include this header better make sure that it hasn't already included Windows headers (or that it has undefined the macros).
Oh, indeed, I hadn't thought about that. I'll test how it behaves if that header is included.


================
Comment at: tools/llvm-rc/ResourceFileWriter.h:33
+  CP_WIN_1252 = 1252, // A codepage where all 8 bit values correspond to
+                      // unicode code points with the same value.
+  CP_UTF8 = 65001,    // UTF-8.
----------------
amccarth wrote:
> That's not _strictly_ true.  Windows-1252 maps many characters in the range 0x80-0x9F that do not correspond to the "upper" control characters Unicode has at the corresponding code points.  For example, Windows-1252 0x83 corresponds to Unicode U+0192 (LATIN SMALL LETTER F WITH HOOK).
> 
> It's probably true enough for practical purposes, but I'd be disappointed if this comment leads readers to not appreciate the differences.
> 
> Also, s/8 bit values/8-bit values/.
> 
> 
Oh, thanks for pointing this out!

Is there any other codepage I should rather pick for the same purpose - trivial implementation in converting to unicode without external dependencies? Win-28591 aka latin1?


Repository:
  rL LLVM

https://reviews.llvm.org/D46238