mstorsjo added inline comments.

================
Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:259
 
-} // anonymous namespace
+static bool consume_back_lower(StringRef &S, const char *str) {
+  if (!S.endswith_lower(str))
----------------
aganea wrote:
> `s/str/Str/`
Thanks, will fix.


================
Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:300
+
+std::string unescape(StringRef S) {
+  std::string Out;
----------------
aganea wrote:
> I would also need this function in D43002 (see unescapeSlashes), do you think 
> we can move it to `sys::path` or any other "support" lib?
This does things differently than your function - this converts `\\\"` into 
`\"` while your version leaves `\\"` if I read it correctly.

(I'm also considering a different, platform specific unescaping strategy - 
that'd be closer to how GNU windres behaves, but makes for a more inconsistent 
tool. Keeping the logic here makes it easier to tweak if needed.)


================
Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:567
+  if (InputArgs.hasArg(OPT_lang_id)) {
+    if (InputArgs.getLastArgValue(OPT_lang_id).getAsInteger(16, Opts.LangId))
+      fatalError("Invalid language id: " +
----------------
aganea wrote:
> There was a latent issue here - unrelated to the moving of code around - I 
> don't know if you want to fix it now or after? That is, `s/16/0/`.
Hmm, as far as I can see, both rc.exe and GNU windres interpret values as 
hexadecimal - if I run e.g. `rc.exe -l 40 test.rc`, where `40` is ambiguous, I 
get language id 64.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100756/new/

https://reviews.llvm.org/D100756

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to