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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/test/tools/llvm-rc/Inputs/tag-user.rc:5
 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,
 }
----------------
amccarth wrote:
> 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.)
> 
> 
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.


================
Comment at: llvm/tools/llvm-rc/ResourceScriptParser.cpp:518
+    // the first one).
+    while (consumeOptionalType(Kind::Comma));
   }
----------------
amccarth wrote:
> The prevailing style for empty loop bodies in LLVM appears to be empty braces `{}`, which I believe will also satisfy the lint check.
Right, I can change that. (I had locally applied the clang-format change after submitting this version of the patch.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105621