This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mstorsjo on Jul 8 2021, 4:12 AM.

Details

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

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Jul 8 2021, 4:12 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2021, 4:12 AM

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 edited the summary of this revision. (Show Details)Jul 8 2021, 10:53 AM
mstorsjo added inline comments.Jul 8 2021, 11:04 AM
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 updated this revision to Diff 357333.Jul 8 2021, 1:03 PM

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 accepted this revision.Jul 8 2021, 4:12 PM

LGTM

This revision is now accepted and ready to land.Jul 8 2021, 4:12 PM
This revision was automatically updated to reflect the committed changes.