[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

516

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

518

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

518

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/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/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 =