[PATCH] D46507: [llvm-rc] Add support for all missing dialog controls

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: tools/llvm-rc/ResourceScriptParser.cpp:484
+    RETURN_IF_ERROR(consumeType(Kind::Comma));
+    ASSIGN_OR_RETURN(Args, readIntsWithCommas(5, 7));
+
----------------
amccarth wrote:
> This is OK for now, but in the future I think you'll need special code for `style` and `exstyle` because those are really expressions that don't work exactly like regular C-style bitwise operations.
> 
> So I'm not sure how much `readIntsWithCommas` is buying versus reading the arguments into well-named variables like `X`, `Y`, `Width`, `Height`, etc., which would then make reading the Control c'tor calls a lot easier and would highlight the difference between the `CONTROL` syntax versus the syntax for the pre-defined control types.
> 
> About a third of this function is special case handling for `CONTROL` when the only differences are two little tweaks to the syntax (position of `style` and the explicit `class` parameter).  It feels like a lot of near-duplicate code (e.g., the TakeOptArg lambda has to be defined twice) to handle the tweaks.
I rewrote this, making it just one main function with minor exceptions for CONTROL vs others, making the similarities more visible, hopefully like you wanted.


================
Comment at: tools/llvm-rc/ResourceScriptStmt.cpp:146
+    {"SCROLLBAR", CtlInfo{0x50000000, ClsScrollBar, false}},
+    {"CONTROL", CtlInfo{0x5000002A, 0, true}},
 };
----------------
mstorsjo wrote:
> amccarth wrote:
> > For `CONTROL`, where does the `0x2A` come from?  I know `0x5000000` is `WS_CHILD | WS_VISIBLE`.
> > 
> > It would be nice to see the styles represented in a way that makes it easier to check the table.
> Oh, that 0x2A must be from the test file itself. Will fix.
I'll leave splitting up the style hardcoded values into symbolic values to a separate patch, since it's already that way.


https://reviews.llvm.org/D46507