[llvm-rc] Refactoring needed for ACCELERATORS and MENU resources serialization (serialization, pt 1.5).

[mnbvmar]

Summary

This:

• Unifies the internal representation of flags in ACCELERATORS and MENU with the corresponding representation in .res files (noticed in https://reviews.llvm.org/D37828#inline-329828).
• Creates an RCResource subclass, OptStatementsRCResource, describing resource statements that can declare resource-local optional statements (proposed in https://reviews.llvm.org/D37824#inline-329775).

These modifications don't fit to any of the current patches, so I'm submitting them as a separate patch.

[ecbeckmann]

This looks good to me.

[rnk]

Looks good, suggestion is optional

llvm/tools/llvm-rc/ResourceScriptStmt.h

217–218 ↗

(On Diff #115149)

nit: It might be nice to fold these together into std::pair<StringRef, uint32_t> or a custom OptionFlag struct.

llvm/trunk/tools/llvm-rc/ResourceScriptParser.h

private:
   // commas. The parser stops reading after fetching MaxCount integers
   // or after an error occurs. Whenever the parser reads a comma, it
   // expects an integer to follow.
   Expected<SmallVector<uint32_t, 8>> readIntsWithCommas(size_t MinCount,
                                                         size_t MaxCount);
 
   // Read an unknown number of flags preceded by commas. Each correct flag
   // has an entry in FlagDesc array of length NumFlags. In case i-th
-  // flag (0-based) has been read, the i-th bit of the result is set.
+  // flag (0-based) has been read, the result is OR-ed with FlagValues[i].
   // As long as parser has a comma to read, it expects to be fed with
   // a correct flag afterwards.
-  Expected<uint32_t> parseFlags(ArrayRef<StringRef> FlagDesc);
+  Expected<uint32_t> parseFlags(ArrayRef<StringRef> FlagDesc,
+                                ArrayRef<uint32_t> FlagValues);
 
   // Reads a set of optional statements. These can change the behavior of
   // a number of resource types (e.g. STRINGTABLE, MENU or DIALOG) if provided
   // before the main block with the contents of the resource.
   // Usually, resources use a basic set of optional statements:
   //    CHARACTERISTICS, LANGUAGE, VERSION
   // However, DIALOG and DIALOGEX extend this list by the following items:
   //    CAPTION, CLASS, EXSTYLE, FONT, MENU, STYLE

llvm/trunk/tools/llvm-rc/ResourceScriptParser.cpp

if (auto IntResult = readInt())
       Result.push_back(*IntResult);
     else
       return FailureHandler(IntResult.takeError());
   }
 
   return std::move(Result);
 }
 
-Expected<uint32_t> RCParser::parseFlags(ArrayRef<StringRef> FlagDesc) {
-  assert(FlagDesc.size() <= 32 && "More than 32 flags won't fit in result.");
+Expected<uint32_t> RCParser::parseFlags(ArrayRef<StringRef> FlagDesc,
+                                        ArrayRef<uint32_t> FlagValues) {
   assert(!FlagDesc.empty());
+  assert(FlagDesc.size() == FlagValues.size());
 
   uint32_t Result = 0;
   while (isNextTokenKind(Kind::Comma)) {
     consume();
     ASSIGN_OR_RETURN(FlagResult, readIdentifier());
     bool FoundFlag = false;
 
     for (size_t FlagId = 0; FlagId < FlagDesc.size(); ++FlagId) {
       if (!FlagResult->equals_lower(FlagDesc[FlagId]))
         continue;
 
-      Result |= (1U << FlagId);
+      Result |= FlagValues[FlagId];
       FoundFlag = true;
       break;
     }
 
     if (!FoundFlag)
       return getExpectedError(join(FlagDesc, "/"), true);
   }
 
RCParser::ParseType RCParser::parseAcceleratorsResource() {
 
   auto Accels =
       llvm::make_unique<AcceleratorsResource>(std::move(*OptStatements));
 
   while (!consumeOptionalType(Kind::BlockEnd)) {
     ASSIGN_OR_RETURN(EventResult, readIntOrString());
     RETURN_IF_ERROR(consumeType(Kind::Comma));
     ASSIGN_OR_RETURN(IDResult, readInt());
-    ASSIGN_OR_RETURN(FlagsResult,
-                     parseFlags(AcceleratorsResource::Accelerator::OptionsStr));
+    ASSIGN_OR_RETURN(
+        FlagsResult,
+        parseFlags(AcceleratorsResource::Accelerator::OptionsStr,
+                   AcceleratorsResource::Accelerator::OptionsFlags));
     Accels->addAccelerator(*EventResult, *IDResult, *FlagsResult);
   }
 
   return std::move(Accels);
 }
 
 RCParser::ParseType RCParser::parseCursorResource() {
   ASSIGN_OR_RETURN(Arg, readString());
while (!consumeOptionalType(Kind::BlockEnd)) {
     uint32_t MenuResult = -1;
 
     if (IsMenuItem) {
       RETURN_IF_ERROR(consumeType(Kind::Comma));
       ASSIGN_OR_RETURN(IntResult, readInt());
       MenuResult = *IntResult;
     }
 
-    ASSIGN_OR_RETURN(FlagsResult, parseFlags(MenuDefinition::OptionsStr));
+    ASSIGN_OR_RETURN(FlagsResult, parseFlags(MenuDefinition::OptionsStr,
+                                             MenuDefinition::OptionsFlags));
 
     if (IsPopup) {
       // If POPUP, read submenu items recursively.
       ASSIGN_OR_RETURN(SubMenuResult, parseMenuItemsList());
       List.addDefinition(llvm::make_unique<PopupItem>(
           *CaptionResult, *FlagsResult, std::move(*SubMenuResult)));
       continue;
     }

llvm/trunk/tools/llvm-rc/ResourceScriptStmt.h

public:
   OptionalStmtList() {}
   virtual raw_ostream &log(raw_ostream &OS) const;
 
   void addStmt(std::unique_ptr<OptionalStmt> Stmt) {
     Statements.push_back(std::move(Stmt));
   }
 };
 
+class OptStatementsRCResource : public RCResource {
+public:
+  std::unique_ptr<OptionalStmtList> OptStatements;
+
+  OptStatementsRCResource(OptionalStmtList &&Stmts)
+      : OptStatements(llvm::make_unique<OptionalStmtList>(std::move(Stmts))) {}
+};
+
 // LANGUAGE statement. It can occur both as a top-level statement (in such
 // a situation, it changes the default language until the end of the file)
 // and as an optional resource statement (then it changes the language
 // of a single resource).
 //
 // Ref: msdn.microsoft.com/en-us/library/windows/desktop/aa381019(v=vs.85).aspx
 class LanguageResource : public OptionalStmt {
 public:
   uint32_t Lang, SubLang;
 
   LanguageResource(uint32_t LangId, uint32_t SubLangId)
       : Lang(LangId), SubLang(SubLangId) {}
   raw_ostream &log(raw_ostream &) const override;
 
   // This is not a regular top-level statement; when it occurs, it just
   // modifies the language context.
   Error visit(Visitor *V) const override { return V->visitLanguageStmt(this); }
   Twine getResourceTypeName() const override { return "LANGUAGE"; }
 };
 
 // ACCELERATORS resource. Defines a named table of accelerators for the app.
 //
 // Ref: msdn.microsoft.com/en-us/library/windows/desktop/aa380610(v=vs.85).aspx
-class AcceleratorsResource : public RCResource {
+class AcceleratorsResource : public OptStatementsRCResource {
 public:
   class Accelerator {
   public:
     IntOrString Event;
     uint32_t Id;
-    uint8_t Flags;
+    uint16_t Flags;
 
     enum Options {
-      ASCII = (1 << 0),
-      VIRTKEY = (1 << 1),
-      NOINVERT = (1 << 2),
-      ALT = (1 << 3),
-      SHIFT = (1 << 4),
-      CONTROL = (1 << 5)
+      // This is actually 0x0000 (accelerator is assumed to be ASCII if it's
+      // not VIRTKEY). However, rc.exe behavior is different in situations
+      // "only ASCII defined" and "neither ASCII nor VIRTKEY defined".
+      // Therefore, we include ASCII as another flag. This must be zeroed
+      // when serialized.
+      ASCII = 0x8000,
+      VIRTKEY = 0x0001,
+      NOINVERT = 0x0002,
+      ALT = 0x0010,
+      SHIFT = 0x0004,
+      CONTROL = 0x0008
     };
 
     static constexpr size_t NumFlags = 6;
     static StringRef OptionsStr[NumFlags];
+    static uint32_t OptionsFlags[NumFlags];
   };
 
-  AcceleratorsResource(OptionalStmtList &&OptStmts)
-      : OptStatements(std::move(OptStmts)) {}
-  void addAccelerator(IntOrString Event, uint32_t Id, uint8_t Flags) {
+  using OptStatementsRCResource::OptStatementsRCResource;
+  void addAccelerator(IntOrString Event, uint32_t Id, uint16_t Flags) {
     Accelerators.push_back(Accelerator{Event, Id, Flags});
   }
   raw_ostream &log(raw_ostream &) const override;
 
 private:
   std::vector<Accelerator> Accelerators;
-  OptionalStmtList OptStatements;
 };
 
 // CURSOR resource. Represents a single cursor (".cur") file.
 //
 // Ref: msdn.microsoft.com/en-us/library/windows/desktop/aa380920(v=vs.85).aspx
 class CursorResource : public RCResource {
   StringRef CursorLoc;
 

 // (usually located in the upper part of the dialog.)
 //
 // Ref: msdn.microsoft.com/en-us/library/windows/desktop/aa381025(v=vs.85).aspx
 
 // Description of a single submenu item.
 class MenuDefinition {
 public:
   enum Options {
-    CHECKED = (1 << 0),
-    GRAYED = (1 << 1),
-    HELP = (1 << 2),
-    INACTIVE = (1 << 3),
-    MENUBARBREAK = (1 << 4),
-    MENUBREAK = (1 << 5)
+    CHECKED = 0x0008,
+    GRAYED = 0x0001,
+    HELP = 0x4000,
+    INACTIVE = 0x0002,
+    MENUBARBREAK = 0x0020,
+    MENUBREAK = 0x0040
   };
 
   static constexpr size_t NumFlags = 6;
   static StringRef OptionsStr[NumFlags];
-  static raw_ostream &logFlags(raw_ostream &, uint8_t Flags);
+  static uint32_t OptionsFlags[NumFlags];
+  static raw_ostream &logFlags(raw_ostream &, uint16_t Flags);
   virtual raw_ostream &log(raw_ostream &OS) const {
     return OS << "Base menu definition\n";
   }
   virtual ~MenuDefinition() {}
 };
 
 // Recursive description of a whole submenu.
 class MenuDefinitionList : public MenuDefinition {

 };
 
 // MENUITEM statement definition.
 //
 // Ref: msdn.microsoft.com/en-us/library/windows/desktop/aa381024(v=vs.85).aspx
 class MenuItem : public MenuDefinition {
   StringRef Name;
   uint32_t Id;
-  uint8_t Flags;
+  uint16_t Flags;
 
 public:
-  MenuItem(StringRef Caption, uint32_t ItemId, uint8_t ItemFlags)
+  MenuItem(StringRef Caption, uint32_t ItemId, uint16_t ItemFlags)
       : Name(Caption), Id(ItemId), Flags(ItemFlags) {}
   raw_ostream &log(raw_ostream &) const override;
 };
 
 // POPUP statement definition.
 //
 // Ref: msdn.microsoft.com/en-us/library/windows/desktop/aa381030(v=vs.85).aspx
 class PopupItem : public MenuDefinition {
   StringRef Name;
-  uint8_t Flags;
+  uint16_t Flags;
   MenuDefinitionList SubItems;
 
 public:
-  PopupItem(StringRef Caption, uint8_t ItemFlags,
+  PopupItem(StringRef Caption, uint16_t ItemFlags,
             MenuDefinitionList &&SubItemsList)
       : Name(Caption), Flags(ItemFlags), SubItems(std::move(SubItemsList)) {}
   raw_ostream &log(raw_ostream &) const override;
 };
 
 // Menu resource definition.
-class MenuResource : public RCResource {
-  OptionalStmtList OptStatements;
+class MenuResource : public OptStatementsRCResource {
   MenuDefinitionList Elements;
 
 public:
   MenuResource(OptionalStmtList &&OptStmts, MenuDefinitionList &&Items)
-      : OptStatements(std::move(OptStmts)), Elements(std::move(Items)) {}
+      : OptStatementsRCResource(std::move(OptStmts)),
+        Elements(std::move(Items)) {}
   raw_ostream &log(raw_ostream &) const override;
 };
 
 // STRINGTABLE resource. Contains a list of strings, each having its unique ID.
 //
 // Ref: msdn.microsoft.com/en-us/library/windows/desktop/aa381050(v=vs.85).aspx
-class StringTableResource : public RCResource {
-  OptionalStmtList OptStatements;
+class StringTableResource : public OptStatementsRCResource {
   std::vector<std::pair<uint32_t, StringRef>> Table;
 
 public:
-  StringTableResource(OptionalStmtList &&OptStmts)
-      : OptStatements(std::move(OptStmts)) {}
+  using OptStatementsRCResource::OptStatementsRCResource;
   void addString(uint32_t ID, StringRef String) {
     Table.emplace_back(ID, String);
   }
   raw_ostream &log(raw_ostream &) const override;
 };
 
 // -- DIALOG(EX) resource and its helper classes --
 //
public:
   static const StringSet<> CtlsWithTitle;
 
   raw_ostream &log(raw_ostream &) const;
 };
 
 // Single dialog definition. We don't create distinct classes for DIALOG and
 // DIALOGEX because of their being too similar to each other. We only have a
 // flag determining the type of the dialog box.
-class DialogResource : public RCResource {
+class DialogResource : public OptStatementsRCResource {
   uint32_t X, Y, Width, Height, HelpID;
-  OptionalStmtList OptStatements;
   std::vector<Control> Controls;
   bool IsExtended;
 
 public:
   DialogResource(uint32_t PosX, uint32_t PosY, uint32_t DlgWidth,
                  uint32_t DlgHeight, uint32_t DlgHelpID,
                  OptionalStmtList &&OptStmts, bool IsDialogEx)
-      : X(PosX), Y(PosY), Width(DlgWidth), Height(DlgHeight), HelpID(DlgHelpID),
-        OptStatements(std::move(OptStmts)), IsExtended(IsDialogEx) {}
+      : OptStatementsRCResource(std::move(OptStmts)), X(PosX), Y(PosY),
+        Width(DlgWidth), Height(DlgHeight), HelpID(DlgHelpID),
+        IsExtended(IsDialogEx) {}
 
   void addControl(Control &&Ctl) { Controls.push_back(std::move(Ctl)); }
 
   raw_ostream &log(raw_ostream &) const override;
 };
 
 // User-defined resource. It is either:
 //   * a link to the file, e.g. NAME TYPE "filename",

llvm/trunk/tools/llvm-rc/ResourceScriptStmt.cpp

 raw_ostream &LanguageResource::log(raw_ostream &OS) const {
   return OS << "Language: " << Lang << ", Sublanguage: " << SubLang << "\n";
 }
 
 StringRef AcceleratorsResource::Accelerator::OptionsStr
     [AcceleratorsResource::Accelerator::NumFlags] = {
         "ASCII", "VIRTKEY", "NOINVERT", "ALT", "SHIFT", "CONTROL"};
 
+uint32_t AcceleratorsResource::Accelerator::OptionsFlags
+    [AcceleratorsResource::Accelerator::NumFlags] = {ASCII, VIRTKEY, NOINVERT,
+                                                     ALT,   SHIFT,   CONTROL};
+
 raw_ostream &AcceleratorsResource::log(raw_ostream &OS) const {
   OS << "Accelerators (" << ResName << "): \n";
-  OptStatements.log(OS);
+  OptStatements->log(OS);
   for (const auto &Acc : Accelerators) {
     OS << "  Accelerator: " << Acc.Event << " " << Acc.Id;
     for (size_t i = 0; i < Accelerator::NumFlags; ++i)
-      if (Acc.Flags & (1U << i))
+      if (Acc.Flags & Accelerator::OptionsFlags[i])
         OS << " " << Accelerator::OptionsStr[i];
     OS << "\n";
   }
   return OS;
 }
 
 raw_ostream &CursorResource::log(raw_ostream &OS) const {
   return OS << "Cursor (" << ResName << "): " << CursorLoc << "\n";
 }
 
 raw_ostream &IconResource::log(raw_ostream &OS) const {
   return OS << "Icon (" << ResName << "): " << IconLoc << "\n";
 }
 
 raw_ostream &HTMLResource::log(raw_ostream &OS) const {
   return OS << "HTML (" << ResName << "): " << HTMLLoc << "\n";
 }
 
 StringRef MenuDefinition::OptionsStr[MenuDefinition::NumFlags] = {
     "CHECKED", "GRAYED", "HELP", "INACTIVE", "MENUBARBREAK", "MENUBREAK"};
 
-raw_ostream &MenuDefinition::logFlags(raw_ostream &OS, uint8_t Flags) {
+uint32_t MenuDefinition::OptionsFlags[MenuDefinition::NumFlags] = {
+    CHECKED, GRAYED, HELP, INACTIVE, MENUBARBREAK, MENUBREAK};
+
+raw_ostream &MenuDefinition::logFlags(raw_ostream &OS, uint16_t Flags) {
   for (size_t i = 0; i < NumFlags; ++i)
-    if (Flags & (1U << i))
+    if (Flags & OptionsFlags[i])
       OS << " " << OptionsStr[i];
   return OS;
 }
 
 raw_ostream &MenuDefinitionList::log(raw_ostream &OS) const {
   OS << "  Menu list starts\n";
   for (auto &Item : Definitions)
     Item->log(OS);
raw_ostream &PopupItem::log(raw_ostream &OS) const {
   OS << "  Popup (" << Name << ")";
   logFlags(OS, Flags);
   OS << ":\n";
   return SubItems.log(OS);
 }
 
 raw_ostream &MenuResource::log(raw_ostream &OS) const {
   OS << "Menu (" << ResName << "):\n";
-  OptStatements.log(OS);
+  OptStatements->log(OS);
   return Elements.log(OS);
 }
 
 raw_ostream &StringTableResource::log(raw_ostream &OS) const {
   OS << "StringTable:\n";
-  OptStatements.log(OS);
+  OptStatements->log(OS);
   for (const auto &String : Table)
     OS << "  " << String.first << " => " << String.second << "\n";
   return OS;
 }
 
 const StringSet<> Control::SupportedCtls = {
     "LTEXT", "RTEXT", "CTEXT", "PUSHBUTTON", "DEFPUSHBUTTON", "EDITTEXT"};
 
if (HelpID)
     OS << ", help ID: " << *HelpID;
   return OS << "\n";
 }
 
 raw_ostream &DialogResource::log(raw_ostream &OS) const {
   OS << "Dialog" << (IsExtended ? "Ex" : "") << " (" << ResName << "): loc: ("
      << X << ", " << Y << "), size: [" << Width << ", " << Height
      << "], help ID: " << HelpID << "\n";
-  OptStatements.log(OS);
+  OptStatements->log(OS);
   for (auto &Ctl : Controls)
     Ctl.log(OS);
   return OS;
 }
 
 raw_ostream &VersionInfoBlock::log(raw_ostream &OS) const {
   OS << "  Start of block (name: " << Name << ")\n";
   for (auto &Stmt : Stmts)