[llvm-rc] Make commas in user data structs optional

[mstorsjo]

Summary

This matches what rc.exe tolerates in this type.

This fixes cases like this:

1 RT_MANIFEST
BEGIN
  "<?xml version=""1.0""?>\n"
  "<assembly>\n"
  "</assembly>\n"
END

[amccarth]

Two small requests, then LGTM.

llvm/test/tools/llvm-rc/Inputs/tag-user.rc

5

Could you add an extra comma between a couple tokens here? The 500/500 example has it, which is a true user-defined "resource type." This one is an RCDATA which should obey the same rules. The implementation is shared, so this should be fine, but if RCDATA is ever treated differently, we'd still want this to work.

BTW: I'm not sure splitting "data" into two strings with no comma actually exercises your change. Since RC (theoretically) works with the output from the preprocessor, I'd expect the preprocessor to concatenate those into one token. (Not a big deal.)

llvm/tools/llvm-rc/ResourceScriptParser.cpp

513

I would have guessed zero-or-one comma, but you're right. RC.EXE allows any number of commas.

515

The prevailing style for empty loop bodies in LLVM appears to be empty braces {}, which I believe will also satisfy the lint check.

[mstorsjo]

llvm/test/tools/llvm-rc/Inputs/tag-user.rc

5

Sure, I can add more commas. The preprocessor doesn’t merge those split string literals, and that’s actually the main practical thing I’m trying to fix here, see the example in the description. (That case doesn’t work before, even when run through the preprocessor before feeding it to llvm-rc.)

Do you want me add a separate testcase with exactly that form too, for clarity? I guess the newlines between the tokens makes it at least slightly different.

llvm/tools/llvm-rc/ResourceScriptParser.cpp

515

Right, I can change that. (I had locally applied the clang-format change after submitting this version of the patch.)

[mstorsjo]

Added more cases of multiple commas in tag-user.rc, changed while(); into while() {}, added a separate standalone testcase of a full embedded manifest file without commas between the string literals.

[amccarth]

LGTM

llvm/test/tools/llvm-rc/Inputs/inline-manifest.rc

+1 24
+BEGIN
+  "<?xml version=""1.0"" encoding=""UTF-8"" standalone=""yes""?>\n"
+  "<assembly xmlns=""urn:schemas-microsoft-com:asm.v1"" manifestVersion=""1.0"">\n"
+  "</assembly>\n"
+END

llvm/test/tools/llvm-rc/Inputs/tag-user.rc

 500 500 {
-  1, 2, 3, 4, 5, "data", L"wide data", 0xABCD, 0xABCDEF01L
+  1, 2 3, ,4, 5, "da", "ta" L"wide data", 0xABCD, 0xABCDEF01L
 }
 501 RCDATA {
-  1, 2, 3, 4, 5, "data", L"wide data", 0xABCD, 0xABCDEF01L
+  1, 2, 3, 4, ,5, "da" "ta" L"wide", L" data", 0xABCD 0xABCDEF01L,
 }
 
 NAME1 NAME2 {}
 
 600 600 "bitmap.bmp"
 

llvm/test/tools/llvm-rc/inline-manifest.test

+; RUN: llvm-rc -no-preprocess /FO %t -- %p/Inputs/inline-manifest.rc
+; RUN: llvm-readobj %t | FileCheck %s
+
+; CHECK:      Resource type (int): MANIFEST (ID 24)
+; CHECK-NEXT: Resource name (int): 1
+; CHECK-NEXT: Data version: 0
+; CHECK-NEXT: Memory flags: 0x30
+; CHECK-NEXT: Language ID: 1033
+; CHECK-NEXT: Version (major): 0
+; CHECK-NEXT: Version (minor): 0
+; CHECK-NEXT: Characteristics: 0
+; CHECK-NEXT: Data size: 142
+; CHECK-NEXT: Data: (
+; CHECK-NEXT:   0000: 3C3F786D 6C207665 7273696F 6E3D2231  |<?xml version="1|
+; CHECK-NEXT:   0010: 2E302220 656E636F 64696E67 3D225554  |.0" encoding="UT|
+; CHECK-NEXT:   0020: 462D3822 20737461 6E64616C 6F6E653D  |F-8" standalone=|
+; CHECK-NEXT:   0030: 22796573 223F3E0A 3C617373 656D626C  |"yes"?>.<assembl|
+; CHECK-NEXT:   0040: 7920786D 6C6E733D 2275726E 3A736368  |y xmlns="urn:sch|
+; CHECK-NEXT:   0050: 656D6173 2D6D6963 726F736F 66742D63  |emas-microsoft-c|
+; CHECK-NEXT:   0060: 6F6D3A61 736D2E76 3122206D 616E6966  |om:asm.v1" manif|
+; CHECK-NEXT:   0070: 65737456 65727369 6F6E3D22 312E3022  |estVersion="1.0"|
+; CHECK-NEXT:   0080: 3E0A3C2F 61737365 6D626C79 3E0A      |>.</assembly>.|
+; CHECK-NEXT: )

llvm/tools/llvm-rc/ResourceScriptParser.cpp

return std::make_unique<UserDefinedResource>(Type, read().value(),
                                                   MemoryFlags);
   default:
     break;
   }
 
   RETURN_IF_ERROR(consumeType(Kind::BlockBegin));
   std::vector<IntOrString> Data;
 
-  // Consume comma before each consecutive token except the first one.
-  bool ConsumeComma = false;
   while (!consumeOptionalType(Kind::BlockEnd)) {
-    if (ConsumeComma)
-      RETURN_IF_ERROR(consumeType(Kind::Comma));
-    ConsumeComma = true;
-
     ASSIGN_OR_RETURN(Item, readIntOrString());
     Data.push_back(*Item);
+
+    // There can be zero or more commas after each token (but not before
+    // the first one).
+    while (consumeOptionalType(Kind::Comma)) {
+    }
   }
 
   return std::make_unique<UserDefinedResource>(Type, std::move(Data),
                                                 MemoryFlags);
 }
 
 RCParser::ParseType RCParser::parseVersionInfoResource() {
   uint16_t MemoryFlags =