[StrTable] Switch the option parser to `llvm::StringTable` #123308

chandlerc · 2025-01-17T09:23:30Z

Now that we have a dedicated abstraction for string tables, switch the option parser library's string table over to it rather than using a raw const char*. Also try to use the StringTable::Offset type rather than a raw unsigned where we can to avoid accidental increments or other issues.

This is based on review feedback for the initial switch of options to a string table. Happy to tweak or adjust if desired here.

Now that we have a dedicated abstraction for string tables, switch the option parser library's string table over to it rather than using a raw `const char*`. Also try to use the `StringTable::Offset` type rather than a raw `unsigned` where we can to avoid accidental increments or other issues. This is based on review feedback for the initial switch of options to a string table. Happy to tweak or adjust if desired here.

llvmbot · 2025-01-17T09:24:07Z

@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-llvm-binary-utilities

Author: Chandler Carruth (chandlerc)

Changes

Now that we have a dedicated abstraction for string tables, switch the option parser library's string table over to it rather than using a raw const char*. Also try to use the StringTable::Offset type rather than a raw unsigned where we can to avoid accidental increments or other issues.

This is based on review feedback for the initial switch of options to a string table. Happy to tweak or adjust if desired here.

Patch is 20.79 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/123308.diff

7 Files Affected:

(modified) clang/lib/Frontend/CompilerInvocation.cpp (+1-1)
(modified) lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp (+2-1)
(modified) llvm/include/llvm/Option/OptTable.h (+38-29)
(modified) llvm/lib/Option/OptTable.cpp (+36-34)
(modified) llvm/tools/llvm-objdump/llvm-objdump.cpp (+2-1)
(modified) llvm/unittests/Option/OptionMarshallingTest.cpp (+2-1)
(modified) llvm/utils/TableGen/OptionParserEmitter.cpp (+7-4)

diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 58658dedbaf1ee..3bf124e4827be9 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -282,7 +282,7 @@ using ArgumentConsumer = CompilerInvocation::ArgumentConsumer;
 #undef OPTTABLE_STR_TABLE_CODE
 
 static llvm::StringRef lookupStrInTable(unsigned Offset) {
-  return &OptionStrTable[Offset];
+  return OptionStrTable[Offset];
 }
 
 #define SIMPLE_ENUM_VALUE_TABLE
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
index 2a36f95c94d0ce..51e9a6d81b8390 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -42,6 +42,7 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Timer.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringTable.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
@@ -1083,7 +1084,7 @@ void PlatformDarwin::AddClangModuleCompilationOptionsForSDKType(
   if (!version.empty() && sdk_type != XcodeSDK::Type::Linux &&
       sdk_type != XcodeSDK::Type::XROS) {
 #define OPTION(PREFIX_OFFSET, NAME_OFFSET, VAR, ...)                           \
-  llvm::StringRef opt_##VAR = &OptionStrTable[NAME_OFFSET];                    \
+  llvm::StringRef opt_##VAR = OptionStrTable[NAME_OFFSET];                     \
   (void)opt_##VAR;
 #include "clang/Driver/Options.inc"
 #undef OPTION
diff --git a/llvm/include/llvm/Option/OptTable.h b/llvm/include/llvm/Option/OptTable.h
index 38a03fef7ae124..61a58aa304ecb4 100644
--- a/llvm/include/llvm/Option/OptTable.h
+++ b/llvm/include/llvm/Option/OptTable.h
@@ -12,6 +12,7 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringTable.h"
 #include "llvm/Option/OptSpecifier.h"
 #include "llvm/Support/StringSaver.h"
 #include <cassert>
@@ -54,7 +55,7 @@ class OptTable {
   /// Entry for a single option instance in the option data table.
   struct Info {
     unsigned PrefixesOffset;
-    unsigned PrefixedNameOffset;
+    StringTable::Offset PrefixedNameOffset;
     const char *HelpText;
     // Help text for specific visibilities. A list of pairs, where each pair
     // is a list of visibilities and a specific help string for those
@@ -80,34 +81,37 @@ class OptTable {
 
     bool hasNoPrefix() const { return PrefixesOffset == 0; }
 
-    unsigned getNumPrefixes(ArrayRef<unsigned> PrefixesTable) const {
-      return PrefixesTable[PrefixesOffset];
+    unsigned getNumPrefixes(ArrayRef<StringTable::Offset> PrefixesTable) const {
+      // We embed the number of prefixes in the value of the first offset.
+      return PrefixesTable[PrefixesOffset].value();
     }
 
-    ArrayRef<unsigned>
-    getPrefixOffsets(ArrayRef<unsigned> PrefixesTable) const {
-      return hasNoPrefix() ? ArrayRef<unsigned>()
+    ArrayRef<StringTable::Offset>
+    getPrefixOffsets(ArrayRef<StringTable::Offset> PrefixesTable) const {
+      return hasNoPrefix() ? ArrayRef<StringTable::Offset>()
                            : PrefixesTable.slice(PrefixesOffset + 1,
                                                  getNumPrefixes(PrefixesTable));
     }
 
-    void appendPrefixes(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+    void appendPrefixes(const StringTable &StrTable,
+                        ArrayRef<StringTable::Offset> PrefixesTable,
                         SmallVectorImpl<StringRef> &Prefixes) const {
-      for (unsigned PrefixOffset : getPrefixOffsets(PrefixesTable))
-        Prefixes.push_back(&StrTable[PrefixOffset]);
+      for (auto PrefixOffset : getPrefixOffsets(PrefixesTable))
+        Prefixes.push_back(StrTable[PrefixOffset]);
     }
 
-    StringRef getPrefix(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+    StringRef getPrefix(const StringTable &StrTable,
+                        ArrayRef<StringTable::Offset> PrefixesTable,
                         unsigned PrefixIndex) const {
-      return &StrTable[getPrefixOffsets(PrefixesTable)[PrefixIndex]];
+      return StrTable[getPrefixOffsets(PrefixesTable)[PrefixIndex]];
     }
 
-    StringRef getPrefixedName(const char *StrTable) const {
-      return &StrTable[PrefixedNameOffset];
+    StringRef getPrefixedName(const StringTable &StrTable) const {
+      return StrTable[PrefixedNameOffset];
     }
 
-    StringRef getName(const char *StrTable,
-                      ArrayRef<unsigned> PrefixesTable) const {
+    StringRef getName(const StringTable &StrTable,
+                      ArrayRef<StringTable::Offset> PrefixesTable) const {
       unsigned PrefixLength =
           hasNoPrefix() ? 0 : getPrefix(StrTable, PrefixesTable, 0).size();
       return getPrefixedName(StrTable).drop_front(PrefixLength);
@@ -117,13 +121,13 @@ class OptTable {
 private:
   // A unified string table for these options. Individual strings are stored as
   // null terminated C-strings at offsets within this table.
-  const char *StrTable;
+  const StringTable *StrTable;
 
   // A table of different sets of prefixes. Each set starts with the number of
   // prefixes in that set followed by that many offsets into the string table
   // for each of the prefix strings. This is essentially a Pascal-string style
   // encoding.
-  ArrayRef<unsigned> PrefixesTable;
+  ArrayRef<StringTable::Offset> PrefixesTable;
 
   /// The option information table.
   ArrayRef<Info> OptionInfos;
@@ -161,7 +165,8 @@ class OptTable {
 protected:
   /// Initialize OptTable using Tablegen'ed OptionInfos. Child class must
   /// manually call \c buildPrefixChars once they are fully constructed.
-  OptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  OptTable(const StringTable &StrTable,
+           ArrayRef<StringTable::Offset> PrefixesTable,
            ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
 
   /// Build (or rebuild) the PrefixChars member.
@@ -171,10 +176,12 @@ class OptTable {
   virtual ~OptTable();
 
   /// Return the string table used for option names.
-  const char *getStrTable() const { return StrTable; }
+  const StringTable &getStrTable() const { return *StrTable; }
 
   /// Return the prefixes table used for option names.
-  ArrayRef<unsigned> getPrefixesTable() const { return PrefixesTable; }
+  ArrayRef<StringTable::Offset> getPrefixesTable() const {
+    return PrefixesTable;
+  }
 
   /// Return the total number of option classes.
   unsigned getNumOptions() const { return OptionInfos.size(); }
@@ -187,25 +194,25 @@ class OptTable {
 
   /// Lookup the name of the given option.
   StringRef getOptionName(OptSpecifier id) const {
-    return getInfo(id).getName(StrTable, PrefixesTable);
+    return getInfo(id).getName(*StrTable, PrefixesTable);
   }
 
   /// Lookup the prefix of the given option.
   StringRef getOptionPrefix(OptSpecifier id) const {
     const Info &I = getInfo(id);
     return I.hasNoPrefix() ? StringRef()
-                           : I.getPrefix(StrTable, PrefixesTable, 0);
+                           : I.getPrefix(*StrTable, PrefixesTable, 0);
   }
 
   void appendOptionPrefixes(OptSpecifier id,
                             SmallVectorImpl<StringRef> &Prefixes) const {
     const Info &I = getInfo(id);
-    I.appendPrefixes(StrTable, PrefixesTable, Prefixes);
+    I.appendPrefixes(*StrTable, PrefixesTable, Prefixes);
   }
 
   /// Lookup the prefixed name of the given option.
   StringRef getOptionPrefixedName(OptSpecifier id) const {
-    return getInfo(id).getPrefixedName(StrTable);
+    return getInfo(id).getPrefixedName(*StrTable);
   }
 
   /// Get the kind of the given option.
@@ -418,19 +425,21 @@ class OptTable {
 /// Specialization of OptTable
 class GenericOptTable : public OptTable {
 protected:
-  GenericOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  GenericOptTable(const StringTable &StrTable,
+                  ArrayRef<StringTable::Offset> PrefixesTable,
                   ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
 };
 
 class PrecomputedOptTable : public OptTable {
 protected:
-  PrecomputedOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  PrecomputedOptTable(const StringTable &StrTable,
+                      ArrayRef<StringTable::Offset> PrefixesTable,
                       ArrayRef<Info> OptionInfos,
-                      ArrayRef<unsigned> PrefixesUnionOffsets,
+                      ArrayRef<StringTable::Offset> PrefixesUnionOffsets,
                       bool IgnoreCase = false)
       : OptTable(StrTable, PrefixesTable, OptionInfos, IgnoreCase) {
-    for (unsigned PrefixOffset : PrefixesUnionOffsets)
-      PrefixesUnion.push_back(&StrTable[PrefixOffset]);
+    for (auto PrefixOffset : PrefixesUnionOffsets)
+      PrefixesUnion.push_back(StrTable[PrefixOffset]);
     buildPrefixChars();
   }
 };
diff --git a/llvm/lib/Option/OptTable.cpp b/llvm/lib/Option/OptTable.cpp
index 87e6f1f12364c2..6d10e6154147ec 100644
--- a/llvm/lib/Option/OptTable.cpp
+++ b/llvm/lib/Option/OptTable.cpp
@@ -33,11 +33,12 @@ using namespace llvm::opt;
 
 namespace {
 struct OptNameLess {
-  const char *StrTable;
-  ArrayRef<unsigned> PrefixesTable;
+  const StringTable *StrTable;
+  ArrayRef<StringTable::Offset> PrefixesTable;
 
-  explicit OptNameLess(const char *StrTable, ArrayRef<unsigned> PrefixesTable)
-      : StrTable(StrTable), PrefixesTable(PrefixesTable) {}
+  explicit OptNameLess(const StringTable &StrTable,
+                       ArrayRef<StringTable::Offset> PrefixesTable)
+      : StrTable(&StrTable), PrefixesTable(PrefixesTable) {}
 
 #ifndef NDEBUG
   inline bool operator()(const OptTable::Info &A,
@@ -45,13 +46,13 @@ struct OptNameLess {
     if (&A == &B)
       return false;
 
-    if (int Cmp = StrCmpOptionName(A.getName(StrTable, PrefixesTable),
-                                   B.getName(StrTable, PrefixesTable)))
+    if (int Cmp = StrCmpOptionName(A.getName(*StrTable, PrefixesTable),
+                                   B.getName(*StrTable, PrefixesTable)))
       return Cmp < 0;
 
     SmallVector<StringRef, 8> APrefixes, BPrefixes;
-    A.appendPrefixes(StrTable, PrefixesTable, APrefixes);
-    B.appendPrefixes(StrTable, PrefixesTable, BPrefixes);
+    A.appendPrefixes(*StrTable, PrefixesTable, APrefixes);
+    B.appendPrefixes(*StrTable, PrefixesTable, BPrefixes);
 
     if (int Cmp = StrCmpOptionPrefixes(APrefixes, BPrefixes))
       return Cmp < 0;
@@ -68,7 +69,7 @@ struct OptNameLess {
   // Support lower_bound between info and an option name.
   inline bool operator()(const OptTable::Info &I, StringRef Name) const {
     // Do not fallback to case sensitive comparison.
-    return StrCmpOptionName(I.getName(StrTable, PrefixesTable), Name, false) <
+    return StrCmpOptionName(I.getName(*StrTable, PrefixesTable), Name, false) <
            0;
   }
 };
@@ -76,9 +77,10 @@ struct OptNameLess {
 
 OptSpecifier::OptSpecifier(const Option *Opt) : ID(Opt->getID()) {}
 
-OptTable::OptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+OptTable::OptTable(const StringTable &StrTable,
+                   ArrayRef<StringTable::Offset> PrefixesTable,
                    ArrayRef<Info> OptionInfos, bool IgnoreCase)
-    : StrTable(StrTable), PrefixesTable(PrefixesTable),
+    : StrTable(&StrTable), PrefixesTable(PrefixesTable),
       OptionInfos(OptionInfos), IgnoreCase(IgnoreCase) {
   // Explicitly zero initialize the error to work around a bug in array
   // value-initialization on MinGW with gcc 4.3.5.
@@ -151,13 +153,13 @@ static bool isInput(const ArrayRef<StringRef> &Prefixes, StringRef Arg) {
 }
 
 /// \returns Matched size. 0 means no match.
-static unsigned matchOption(const char *StrTable,
-                            ArrayRef<unsigned> PrefixesTable,
+static unsigned matchOption(const StringTable &StrTable,
+                            ArrayRef<StringTable::Offset> PrefixesTable,
                             const OptTable::Info *I, StringRef Str,
                             bool IgnoreCase) {
   StringRef Name = I->getName(StrTable, PrefixesTable);
-  for (unsigned PrefixOffset : I->getPrefixOffsets(PrefixesTable)) {
-    StringRef Prefix = &StrTable[PrefixOffset];
+  for (auto PrefixOffset : I->getPrefixOffsets(PrefixesTable)) {
+    StringRef Prefix = StrTable[PrefixOffset];
     if (Str.starts_with(Prefix)) {
       StringRef Rest = Str.substr(Prefix.size());
       bool Matched = IgnoreCase ? Rest.starts_with_insensitive(Name)
@@ -170,13 +172,13 @@ static unsigned matchOption(const char *StrTable,
 }
 
 // Returns true if one of the Prefixes + In.Names matches Option
-static bool optionMatches(const char *StrTable,
-                          ArrayRef<unsigned> PrefixesTable,
+static bool optionMatches(const StringTable &StrTable,
+                          ArrayRef<StringTable::Offset> PrefixesTable,
                           const OptTable::Info &In, StringRef Option) {
   StringRef Name = In.getName(StrTable, PrefixesTable);
   if (Option.consume_back(Name))
-    for (unsigned PrefixOffset : In.getPrefixOffsets(PrefixesTable))
-      if (Option == &StrTable[PrefixOffset])
+    for (auto PrefixOffset : In.getPrefixOffsets(PrefixesTable))
+      if (Option == StrTable[PrefixOffset])
         return true;
   return false;
 }
@@ -189,7 +191,7 @@ OptTable::suggestValueCompletions(StringRef Option, StringRef Arg) const {
   // Search all options and return possible values.
   for (size_t I = FirstSearchableIndex, E = OptionInfos.size(); I < E; I++) {
     const Info &In = OptionInfos[I];
-    if (!In.Values || !optionMatches(StrTable, PrefixesTable, In, Option))
+    if (!In.Values || !optionMatches(*StrTable, PrefixesTable, In, Option))
       continue;
 
     SmallVector<StringRef, 8> Candidates;
@@ -217,9 +219,9 @@ OptTable::findByPrefix(StringRef Cur, Visibility VisibilityMask,
     if (In.Flags & DisableFlags)
       continue;
 
-    StringRef Name = In.getName(StrTable, PrefixesTable);
-    for (unsigned PrefixOffset : In.getPrefixOffsets(PrefixesTable)) {
-      StringRef Prefix = &StrTable[PrefixOffset];
+    StringRef Name = In.getName(*StrTable, PrefixesTable);
+    for (auto PrefixOffset : In.getPrefixOffsets(PrefixesTable)) {
+      StringRef Prefix = (*StrTable)[PrefixOffset];
       std::string S = (Twine(Prefix) + Name + "\t").str();
       if (In.HelpText)
         S += In.HelpText;
@@ -271,7 +273,7 @@ unsigned OptTable::internalFindNearest(
 
   for (const Info &CandidateInfo :
        ArrayRef<Info>(OptionInfos).drop_front(FirstSearchableIndex)) {
-    StringRef CandidateName = CandidateInfo.getName(StrTable, PrefixesTable);
+    StringRef CandidateName = CandidateInfo.getName(*StrTable, PrefixesTable);
 
     // We can eliminate some option prefix/name pairs as candidates right away:
     // * Ignore option candidates with empty names, such as "--", or names
@@ -304,9 +306,9 @@ unsigned OptTable::internalFindNearest(
     // Consider each possible prefix for each candidate to find the most
     // appropriate one. For example, if a user asks for "--helm", suggest
     // "--help" over "-help".
-    for (unsigned CandidatePrefixOffset :
+    for (auto CandidatePrefixOffset :
          CandidateInfo.getPrefixOffsets(PrefixesTable)) {
-      StringRef CandidatePrefix = &StrTable[CandidatePrefixOffset];
+      StringRef CandidatePrefix = (*StrTable)[CandidatePrefixOffset];
       // If Candidate and NormalizedName have more than 'BestDistance'
       // characters of difference, no need to compute the edit distance, it's
       // going to be greater than BestDistance. Don't bother computing Candidate
@@ -359,14 +361,14 @@ std::unique_ptr<Arg> OptTable::parseOneArgGrouped(InputArgList &Args,
   StringRef Name = Str.ltrim(PrefixChars);
   const Info *Start =
       std::lower_bound(OptionInfos.data() + FirstSearchableIndex, End, Name,
-                       OptNameLess(StrTable, PrefixesTable));
+                       OptNameLess(*StrTable, PrefixesTable));
   const Info *Fallback = nullptr;
   unsigned Prev = Index;
 
   // Search for the option which matches Str.
   for (; Start != End; ++Start) {
     unsigned ArgSize =
-        matchOption(StrTable, PrefixesTable, Start, Str, IgnoreCase);
+        matchOption(*StrTable, PrefixesTable, Start, Str, IgnoreCase);
     if (!ArgSize)
       continue;
 
@@ -449,7 +451,7 @@ std::unique_ptr<Arg> OptTable::internalParseOneArg(
 
   // Search for the first next option which could be a prefix.
   Start =
-      std::lower_bound(Start, End, Name, OptNameLess(StrTable, PrefixesTable));
+      std::lower_bound(Start, End, Name, OptNameLess(*StrTable, PrefixesTable));
 
   // Options are stored in sorted order, with '\0' at the end of the
   // alphabet. Since the only options which can accept a string must
@@ -464,7 +466,7 @@ std::unique_ptr<Arg> OptTable::internalParseOneArg(
     // Scan for first option which is a proper prefix.
     for (; Start != End; ++Start)
       if ((ArgSize =
-               matchOption(StrTable, PrefixesTable, Start, Str, IgnoreCase)))
+               matchOption(*StrTable, PrefixesTable, Start, Str, IgnoreCase)))
         break;
     if (Start == End)
       break;
@@ -787,15 +789,15 @@ void OptTable::internalPrintHelp(
   OS.flush();
 }
 
-GenericOptTable::GenericOptTable(const char *StrTable,
-                                 ArrayRef<unsigned> PrefixesTable,
+GenericOptTable::GenericOptTable(const StringTable &StrTable,
+                                 ArrayRef<StringTable::Offset> PrefixesTable,
                                  ArrayRef<Info> OptionInfos, bool IgnoreCase)
     : OptTable(StrTable, PrefixesTable, OptionInfos, IgnoreCase) {
 
   std::set<StringRef> TmpPrefixesUnion;
   for (auto const &Info : OptionInfos.drop_front(FirstSearchableIndex))
-    for (unsigned PrefixOffset : Info.getPrefixOffsets(PrefixesTable))
-      TmpPrefixesUnion.insert(StringRef(&StrTable[PrefixOffset]));
+    for (auto PrefixOffset : Info.getPrefixOffsets(PrefixesTable))
+      TmpPrefixesUnion.insert(StrTable[PrefixOffset]);
   PrefixesUnion.append(TmpPrefixesUnion.begin(), TmpPrefixesUnion.end());
   buildPrefixChars();
 }
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index 93fed8ee8e6f42..99e0440dce78d5 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -94,7 +94,8 @@ namespace {
 
 class CommonOptTable : public opt::GenericOptTable {
 public:
-  CommonOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  CommonOptTable(const StringTable &StrTable,
+                 ArrayRef<StringTable::Offset> PrefixesTable,
                  ArrayRef<Info> OptionInfos, const char *Usage,
                  const char *Description)
       : opt::GenericOptTable(StrTable, PrefixesTable, OptionInfos),
diff --git a/llvm/unittests/Option/OptionMarshallingTest.cpp b/llvm/unittests/Option/OptionMarshallingTest.cpp
index 08c3b019689f8c..005144b91bf7f3 100644
--- a/llvm/unittests/Option/OptionMarshallingTest.cpp
+++ b/llvm/unittests/Option/OptionMarshallingTest.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringTable.h"
 #include "gtest/gtest.h"
 
 #define OPTTABLE_STR_TABLE_CODE
@@ -20,7 +21,7 @@ struct OptionWithMarshallingInfo {
   const char *ImpliedValue;
 
   llvm::StringRef getPrefixedName() const {
-    return &OptionStrTable[PrefixedNameOffset];
+    return OptionStrTable[PrefixedNameOffset];
   }
 };
 
diff --git a/llvm/utils/TableGen/OptionParserEmitter.cpp b/llvm/utils/TableGen/OptionParserEmitter.cpp
index 8b92d252392194..35a452890b0ec7 100644
--- a/llvm/utils/TableGen/OptionParserEmitter.cpp
+++ b/llvm/utils/TableGen/OptionParserEmitter.cpp
@@ -303,15 +303,17 @@ static void emitOptionParser(const RecordKeeper &Records, raw_ostream &OS) {
   OS << "/////////\n";
   OS << "// String table\n\n";
   OS << "#ifdef OPTTABLE_STR_TABLE_CODE\n";
-  Table.EmitStringLiteralDef(OS, "static constexpr char OptionStrTable[]",
-                             /*Indent=*/"");
+  Table.EmitStringLiteralDef(
+      OS, "static constexpr llvm::StringTable OptionStrTable",
+      /*Indent=*/"");
   OS << "#endif // OPTTABLE_STR_TABLE_CODE\n\n";
 
   // ...
[truncated]

llvmbot · 2025-01-17T09:24:07Z

@llvm/pr-subscribers-clang

Author: Chandler Carruth (chandlerc)

Changes

Now that we have a dedicated abstraction for string tables, switch the option parser library's string table over to it rather than using a raw const char*. Also try to use the StringTable::Offset type rather than a raw unsigned where we can to avoid accidental increments or other issues.

This is based on review feedback for the initial switch of options to a string table. Happy to tweak or adjust if desired here.

Patch is 20.79 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/123308.diff

7 Files Affected:

(modified) clang/lib/Frontend/CompilerInvocation.cpp (+1-1)
(modified) lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp (+2-1)
(modified) llvm/include/llvm/Option/OptTable.h (+38-29)
(modified) llvm/lib/Option/OptTable.cpp (+36-34)
(modified) llvm/tools/llvm-objdump/llvm-objdump.cpp (+2-1)
(modified) llvm/unittests/Option/OptionMarshallingTest.cpp (+2-1)
(modified) llvm/utils/TableGen/OptionParserEmitter.cpp (+7-4)

diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 58658dedbaf1ee..3bf124e4827be9 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -282,7 +282,7 @@ using ArgumentConsumer = CompilerInvocation::ArgumentConsumer;
 #undef OPTTABLE_STR_TABLE_CODE
 
 static llvm::StringRef lookupStrInTable(unsigned Offset) {
-  return &OptionStrTable[Offset];
+  return OptionStrTable[Offset];
 }
 
 #define SIMPLE_ENUM_VALUE_TABLE
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
index 2a36f95c94d0ce..51e9a6d81b8390 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -42,6 +42,7 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Timer.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringTable.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
@@ -1083,7 +1084,7 @@ void PlatformDarwin::AddClangModuleCompilationOptionsForSDKType(
   if (!version.empty() && sdk_type != XcodeSDK::Type::Linux &&
       sdk_type != XcodeSDK::Type::XROS) {
 #define OPTION(PREFIX_OFFSET, NAME_OFFSET, VAR, ...)                           \
-  llvm::StringRef opt_##VAR = &OptionStrTable[NAME_OFFSET];                    \
+  llvm::StringRef opt_##VAR = OptionStrTable[NAME_OFFSET];                     \
   (void)opt_##VAR;
 #include "clang/Driver/Options.inc"
 #undef OPTION
diff --git a/llvm/include/llvm/Option/OptTable.h b/llvm/include/llvm/Option/OptTable.h
index 38a03fef7ae124..61a58aa304ecb4 100644
--- a/llvm/include/llvm/Option/OptTable.h
+++ b/llvm/include/llvm/Option/OptTable.h
@@ -12,6 +12,7 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringTable.h"
 #include "llvm/Option/OptSpecifier.h"
 #include "llvm/Support/StringSaver.h"
 #include <cassert>
@@ -54,7 +55,7 @@ class OptTable {
   /// Entry for a single option instance in the option data table.
   struct Info {
     unsigned PrefixesOffset;
-    unsigned PrefixedNameOffset;
+    StringTable::Offset PrefixedNameOffset;
     const char *HelpText;
     // Help text for specific visibilities. A list of pairs, where each pair
     // is a list of visibilities and a specific help string for those
@@ -80,34 +81,37 @@ class OptTable {
 
     bool hasNoPrefix() const { return PrefixesOffset == 0; }
 
-    unsigned getNumPrefixes(ArrayRef<unsigned> PrefixesTable) const {
-      return PrefixesTable[PrefixesOffset];
+    unsigned getNumPrefixes(ArrayRef<StringTable::Offset> PrefixesTable) const {
+      // We embed the number of prefixes in the value of the first offset.
+      return PrefixesTable[PrefixesOffset].value();
     }
 
-    ArrayRef<unsigned>
-    getPrefixOffsets(ArrayRef<unsigned> PrefixesTable) const {
-      return hasNoPrefix() ? ArrayRef<unsigned>()
+    ArrayRef<StringTable::Offset>
+    getPrefixOffsets(ArrayRef<StringTable::Offset> PrefixesTable) const {
+      return hasNoPrefix() ? ArrayRef<StringTable::Offset>()
                            : PrefixesTable.slice(PrefixesOffset + 1,
                                                  getNumPrefixes(PrefixesTable));
     }
 
-    void appendPrefixes(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+    void appendPrefixes(const StringTable &StrTable,
+                        ArrayRef<StringTable::Offset> PrefixesTable,
                         SmallVectorImpl<StringRef> &Prefixes) const {
-      for (unsigned PrefixOffset : getPrefixOffsets(PrefixesTable))
-        Prefixes.push_back(&StrTable[PrefixOffset]);
+      for (auto PrefixOffset : getPrefixOffsets(PrefixesTable))
+        Prefixes.push_back(StrTable[PrefixOffset]);
     }
 
-    StringRef getPrefix(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+    StringRef getPrefix(const StringTable &StrTable,
+                        ArrayRef<StringTable::Offset> PrefixesTable,
                         unsigned PrefixIndex) const {
-      return &StrTable[getPrefixOffsets(PrefixesTable)[PrefixIndex]];
+      return StrTable[getPrefixOffsets(PrefixesTable)[PrefixIndex]];
     }
 
-    StringRef getPrefixedName(const char *StrTable) const {
-      return &StrTable[PrefixedNameOffset];
+    StringRef getPrefixedName(const StringTable &StrTable) const {
+      return StrTable[PrefixedNameOffset];
     }
 
-    StringRef getName(const char *StrTable,
-                      ArrayRef<unsigned> PrefixesTable) const {
+    StringRef getName(const StringTable &StrTable,
+                      ArrayRef<StringTable::Offset> PrefixesTable) const {
       unsigned PrefixLength =
           hasNoPrefix() ? 0 : getPrefix(StrTable, PrefixesTable, 0).size();
       return getPrefixedName(StrTable).drop_front(PrefixLength);
@@ -117,13 +121,13 @@ class OptTable {
 private:
   // A unified string table for these options. Individual strings are stored as
   // null terminated C-strings at offsets within this table.
-  const char *StrTable;
+  const StringTable *StrTable;
 
   // A table of different sets of prefixes. Each set starts with the number of
   // prefixes in that set followed by that many offsets into the string table
   // for each of the prefix strings. This is essentially a Pascal-string style
   // encoding.
-  ArrayRef<unsigned> PrefixesTable;
+  ArrayRef<StringTable::Offset> PrefixesTable;
 
   /// The option information table.
   ArrayRef<Info> OptionInfos;
@@ -161,7 +165,8 @@ class OptTable {
 protected:
   /// Initialize OptTable using Tablegen'ed OptionInfos. Child class must
   /// manually call \c buildPrefixChars once they are fully constructed.
-  OptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  OptTable(const StringTable &StrTable,
+           ArrayRef<StringTable::Offset> PrefixesTable,
            ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
 
   /// Build (or rebuild) the PrefixChars member.
@@ -171,10 +176,12 @@ class OptTable {
   virtual ~OptTable();
 
   /// Return the string table used for option names.
-  const char *getStrTable() const { return StrTable; }
+  const StringTable &getStrTable() const { return *StrTable; }
 
   /// Return the prefixes table used for option names.
-  ArrayRef<unsigned> getPrefixesTable() const { return PrefixesTable; }
+  ArrayRef<StringTable::Offset> getPrefixesTable() const {
+    return PrefixesTable;
+  }
 
   /// Return the total number of option classes.
   unsigned getNumOptions() const { return OptionInfos.size(); }
@@ -187,25 +194,25 @@ class OptTable {
 
   /// Lookup the name of the given option.
   StringRef getOptionName(OptSpecifier id) const {
-    return getInfo(id).getName(StrTable, PrefixesTable);
+    return getInfo(id).getName(*StrTable, PrefixesTable);
   }
 
   /// Lookup the prefix of the given option.
   StringRef getOptionPrefix(OptSpecifier id) const {
     const Info &I = getInfo(id);
     return I.hasNoPrefix() ? StringRef()
-                           : I.getPrefix(StrTable, PrefixesTable, 0);
+                           : I.getPrefix(*StrTable, PrefixesTable, 0);
   }
 
   void appendOptionPrefixes(OptSpecifier id,
                             SmallVectorImpl<StringRef> &Prefixes) const {
     const Info &I = getInfo(id);
-    I.appendPrefixes(StrTable, PrefixesTable, Prefixes);
+    I.appendPrefixes(*StrTable, PrefixesTable, Prefixes);
   }
 
   /// Lookup the prefixed name of the given option.
   StringRef getOptionPrefixedName(OptSpecifier id) const {
-    return getInfo(id).getPrefixedName(StrTable);
+    return getInfo(id).getPrefixedName(*StrTable);
   }
 
   /// Get the kind of the given option.
@@ -418,19 +425,21 @@ class OptTable {
 /// Specialization of OptTable
 class GenericOptTable : public OptTable {
 protected:
-  GenericOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  GenericOptTable(const StringTable &StrTable,
+                  ArrayRef<StringTable::Offset> PrefixesTable,
                   ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
 };
 
 class PrecomputedOptTable : public OptTable {
 protected:
-  PrecomputedOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  PrecomputedOptTable(const StringTable &StrTable,
+                      ArrayRef<StringTable::Offset> PrefixesTable,
                       ArrayRef<Info> OptionInfos,
-                      ArrayRef<unsigned> PrefixesUnionOffsets,
+                      ArrayRef<StringTable::Offset> PrefixesUnionOffsets,
                       bool IgnoreCase = false)
       : OptTable(StrTable, PrefixesTable, OptionInfos, IgnoreCase) {
-    for (unsigned PrefixOffset : PrefixesUnionOffsets)
-      PrefixesUnion.push_back(&StrTable[PrefixOffset]);
+    for (auto PrefixOffset : PrefixesUnionOffsets)
+      PrefixesUnion.push_back(StrTable[PrefixOffset]);
     buildPrefixChars();
   }
 };
diff --git a/llvm/lib/Option/OptTable.cpp b/llvm/lib/Option/OptTable.cpp
index 87e6f1f12364c2..6d10e6154147ec 100644
--- a/llvm/lib/Option/OptTable.cpp
+++ b/llvm/lib/Option/OptTable.cpp
@@ -33,11 +33,12 @@ using namespace llvm::opt;
 
 namespace {
 struct OptNameLess {
-  const char *StrTable;
-  ArrayRef<unsigned> PrefixesTable;
+  const StringTable *StrTable;
+  ArrayRef<StringTable::Offset> PrefixesTable;
 
-  explicit OptNameLess(const char *StrTable, ArrayRef<unsigned> PrefixesTable)
-      : StrTable(StrTable), PrefixesTable(PrefixesTable) {}
+  explicit OptNameLess(const StringTable &StrTable,
+                       ArrayRef<StringTable::Offset> PrefixesTable)
+      : StrTable(&StrTable), PrefixesTable(PrefixesTable) {}
 
 #ifndef NDEBUG
   inline bool operator()(const OptTable::Info &A,
@@ -45,13 +46,13 @@ struct OptNameLess {
     if (&A == &B)
       return false;
 
-    if (int Cmp = StrCmpOptionName(A.getName(StrTable, PrefixesTable),
-                                   B.getName(StrTable, PrefixesTable)))
+    if (int Cmp = StrCmpOptionName(A.getName(*StrTable, PrefixesTable),
+                                   B.getName(*StrTable, PrefixesTable)))
       return Cmp < 0;
 
     SmallVector<StringRef, 8> APrefixes, BPrefixes;
-    A.appendPrefixes(StrTable, PrefixesTable, APrefixes);
-    B.appendPrefixes(StrTable, PrefixesTable, BPrefixes);
+    A.appendPrefixes(*StrTable, PrefixesTable, APrefixes);
+    B.appendPrefixes(*StrTable, PrefixesTable, BPrefixes);
 
     if (int Cmp = StrCmpOptionPrefixes(APrefixes, BPrefixes))
       return Cmp < 0;
@@ -68,7 +69,7 @@ struct OptNameLess {
   // Support lower_bound between info and an option name.
   inline bool operator()(const OptTable::Info &I, StringRef Name) const {
     // Do not fallback to case sensitive comparison.
-    return StrCmpOptionName(I.getName(StrTable, PrefixesTable), Name, false) <
+    return StrCmpOptionName(I.getName(*StrTable, PrefixesTable), Name, false) <
            0;
   }
 };
@@ -76,9 +77,10 @@ struct OptNameLess {
 
 OptSpecifier::OptSpecifier(const Option *Opt) : ID(Opt->getID()) {}
 
-OptTable::OptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+OptTable::OptTable(const StringTable &StrTable,
+                   ArrayRef<StringTable::Offset> PrefixesTable,
                    ArrayRef<Info> OptionInfos, bool IgnoreCase)
-    : StrTable(StrTable), PrefixesTable(PrefixesTable),
+    : StrTable(&StrTable), PrefixesTable(PrefixesTable),
       OptionInfos(OptionInfos), IgnoreCase(IgnoreCase) {
   // Explicitly zero initialize the error to work around a bug in array
   // value-initialization on MinGW with gcc 4.3.5.
@@ -151,13 +153,13 @@ static bool isInput(const ArrayRef<StringRef> &Prefixes, StringRef Arg) {
 }
 
 /// \returns Matched size. 0 means no match.
-static unsigned matchOption(const char *StrTable,
-                            ArrayRef<unsigned> PrefixesTable,
+static unsigned matchOption(const StringTable &StrTable,
+                            ArrayRef<StringTable::Offset> PrefixesTable,
                             const OptTable::Info *I, StringRef Str,
                             bool IgnoreCase) {
   StringRef Name = I->getName(StrTable, PrefixesTable);
-  for (unsigned PrefixOffset : I->getPrefixOffsets(PrefixesTable)) {
-    StringRef Prefix = &StrTable[PrefixOffset];
+  for (auto PrefixOffset : I->getPrefixOffsets(PrefixesTable)) {
+    StringRef Prefix = StrTable[PrefixOffset];
     if (Str.starts_with(Prefix)) {
       StringRef Rest = Str.substr(Prefix.size());
       bool Matched = IgnoreCase ? Rest.starts_with_insensitive(Name)
@@ -170,13 +172,13 @@ static unsigned matchOption(const char *StrTable,
 }
 
 // Returns true if one of the Prefixes + In.Names matches Option
-static bool optionMatches(const char *StrTable,
-                          ArrayRef<unsigned> PrefixesTable,
+static bool optionMatches(const StringTable &StrTable,
+                          ArrayRef<StringTable::Offset> PrefixesTable,
                           const OptTable::Info &In, StringRef Option) {
   StringRef Name = In.getName(StrTable, PrefixesTable);
   if (Option.consume_back(Name))
-    for (unsigned PrefixOffset : In.getPrefixOffsets(PrefixesTable))
-      if (Option == &StrTable[PrefixOffset])
+    for (auto PrefixOffset : In.getPrefixOffsets(PrefixesTable))
+      if (Option == StrTable[PrefixOffset])
         return true;
   return false;
 }
@@ -189,7 +191,7 @@ OptTable::suggestValueCompletions(StringRef Option, StringRef Arg) const {
   // Search all options and return possible values.
   for (size_t I = FirstSearchableIndex, E = OptionInfos.size(); I < E; I++) {
     const Info &In = OptionInfos[I];
-    if (!In.Values || !optionMatches(StrTable, PrefixesTable, In, Option))
+    if (!In.Values || !optionMatches(*StrTable, PrefixesTable, In, Option))
       continue;
 
     SmallVector<StringRef, 8> Candidates;
@@ -217,9 +219,9 @@ OptTable::findByPrefix(StringRef Cur, Visibility VisibilityMask,
     if (In.Flags & DisableFlags)
       continue;
 
-    StringRef Name = In.getName(StrTable, PrefixesTable);
-    for (unsigned PrefixOffset : In.getPrefixOffsets(PrefixesTable)) {
-      StringRef Prefix = &StrTable[PrefixOffset];
+    StringRef Name = In.getName(*StrTable, PrefixesTable);
+    for (auto PrefixOffset : In.getPrefixOffsets(PrefixesTable)) {
+      StringRef Prefix = (*StrTable)[PrefixOffset];
       std::string S = (Twine(Prefix) + Name + "\t").str();
       if (In.HelpText)
         S += In.HelpText;
@@ -271,7 +273,7 @@ unsigned OptTable::internalFindNearest(
 
   for (const Info &CandidateInfo :
        ArrayRef<Info>(OptionInfos).drop_front(FirstSearchableIndex)) {
-    StringRef CandidateName = CandidateInfo.getName(StrTable, PrefixesTable);
+    StringRef CandidateName = CandidateInfo.getName(*StrTable, PrefixesTable);
 
     // We can eliminate some option prefix/name pairs as candidates right away:
     // * Ignore option candidates with empty names, such as "--", or names
@@ -304,9 +306,9 @@ unsigned OptTable::internalFindNearest(
     // Consider each possible prefix for each candidate to find the most
     // appropriate one. For example, if a user asks for "--helm", suggest
     // "--help" over "-help".
-    for (unsigned CandidatePrefixOffset :
+    for (auto CandidatePrefixOffset :
          CandidateInfo.getPrefixOffsets(PrefixesTable)) {
-      StringRef CandidatePrefix = &StrTable[CandidatePrefixOffset];
+      StringRef CandidatePrefix = (*StrTable)[CandidatePrefixOffset];
       // If Candidate and NormalizedName have more than 'BestDistance'
       // characters of difference, no need to compute the edit distance, it's
       // going to be greater than BestDistance. Don't bother computing Candidate
@@ -359,14 +361,14 @@ std::unique_ptr<Arg> OptTable::parseOneArgGrouped(InputArgList &Args,
   StringRef Name = Str.ltrim(PrefixChars);
   const Info *Start =
       std::lower_bound(OptionInfos.data() + FirstSearchableIndex, End, Name,
-                       OptNameLess(StrTable, PrefixesTable));
+                       OptNameLess(*StrTable, PrefixesTable));
   const Info *Fallback = nullptr;
   unsigned Prev = Index;
 
   // Search for the option which matches Str.
   for (; Start != End; ++Start) {
     unsigned ArgSize =
-        matchOption(StrTable, PrefixesTable, Start, Str, IgnoreCase);
+        matchOption(*StrTable, PrefixesTable, Start, Str, IgnoreCase);
     if (!ArgSize)
       continue;
 
@@ -449,7 +451,7 @@ std::unique_ptr<Arg> OptTable::internalParseOneArg(
 
   // Search for the first next option which could be a prefix.
   Start =
-      std::lower_bound(Start, End, Name, OptNameLess(StrTable, PrefixesTable));
+      std::lower_bound(Start, End, Name, OptNameLess(*StrTable, PrefixesTable));
 
   // Options are stored in sorted order, with '\0' at the end of the
   // alphabet. Since the only options which can accept a string must
@@ -464,7 +466,7 @@ std::unique_ptr<Arg> OptTable::internalParseOneArg(
     // Scan for first option which is a proper prefix.
     for (; Start != End; ++Start)
       if ((ArgSize =
-               matchOption(StrTable, PrefixesTable, Start, Str, IgnoreCase)))
+               matchOption(*StrTable, PrefixesTable, Start, Str, IgnoreCase)))
         break;
     if (Start == End)
       break;
@@ -787,15 +789,15 @@ void OptTable::internalPrintHelp(
   OS.flush();
 }
 
-GenericOptTable::GenericOptTable(const char *StrTable,
-                                 ArrayRef<unsigned> PrefixesTable,
+GenericOptTable::GenericOptTable(const StringTable &StrTable,
+                                 ArrayRef<StringTable::Offset> PrefixesTable,
                                  ArrayRef<Info> OptionInfos, bool IgnoreCase)
     : OptTable(StrTable, PrefixesTable, OptionInfos, IgnoreCase) {
 
   std::set<StringRef> TmpPrefixesUnion;
   for (auto const &Info : OptionInfos.drop_front(FirstSearchableIndex))
-    for (unsigned PrefixOffset : Info.getPrefixOffsets(PrefixesTable))
-      TmpPrefixesUnion.insert(StringRef(&StrTable[PrefixOffset]));
+    for (auto PrefixOffset : Info.getPrefixOffsets(PrefixesTable))
+      TmpPrefixesUnion.insert(StrTable[PrefixOffset]);
   PrefixesUnion.append(TmpPrefixesUnion.begin(), TmpPrefixesUnion.end());
   buildPrefixChars();
 }
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index 93fed8ee8e6f42..99e0440dce78d5 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -94,7 +94,8 @@ namespace {
 
 class CommonOptTable : public opt::GenericOptTable {
 public:
-  CommonOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  CommonOptTable(const StringTable &StrTable,
+                 ArrayRef<StringTable::Offset> PrefixesTable,
                  ArrayRef<Info> OptionInfos, const char *Usage,
                  const char *Description)
       : opt::GenericOptTable(StrTable, PrefixesTable, OptionInfos),
diff --git a/llvm/unittests/Option/OptionMarshallingTest.cpp b/llvm/unittests/Option/OptionMarshallingTest.cpp
index 08c3b019689f8c..005144b91bf7f3 100644
--- a/llvm/unittests/Option/OptionMarshallingTest.cpp
+++ b/llvm/unittests/Option/OptionMarshallingTest.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringTable.h"
 #include "gtest/gtest.h"
 
 #define OPTTABLE_STR_TABLE_CODE
@@ -20,7 +21,7 @@ struct OptionWithMarshallingInfo {
   const char *ImpliedValue;
 
   llvm::StringRef getPrefixedName() const {
-    return &OptionStrTable[PrefixedNameOffset];
+    return OptionStrTable[PrefixedNameOffset];
   }
 };
 
diff --git a/llvm/utils/TableGen/OptionParserEmitter.cpp b/llvm/utils/TableGen/OptionParserEmitter.cpp
index 8b92d252392194..35a452890b0ec7 100644
--- a/llvm/utils/TableGen/OptionParserEmitter.cpp
+++ b/llvm/utils/TableGen/OptionParserEmitter.cpp
@@ -303,15 +303,17 @@ static void emitOptionParser(const RecordKeeper &Records, raw_ostream &OS) {
   OS << "/////////\n";
   OS << "// String table\n\n";
   OS << "#ifdef OPTTABLE_STR_TABLE_CODE\n";
-  Table.EmitStringLiteralDef(OS, "static constexpr char OptionStrTable[]",
-                             /*Indent=*/"");
+  Table.EmitStringLiteralDef(
+      OS, "static constexpr llvm::StringTable OptionStrTable",
+      /*Indent=*/"");
   OS << "#endif // OPTTABLE_STR_TABLE_CODE\n\n";
 
   // ...
[truncated]

llvmbot · 2025-01-17T09:24:07Z

@llvm/pr-subscribers-tablegen

Author: Chandler Carruth (chandlerc)

Changes

Now that we have a dedicated abstraction for string tables, switch the option parser library's string table over to it rather than using a raw const char*. Also try to use the StringTable::Offset type rather than a raw unsigned where we can to avoid accidental increments or other issues.

This is based on review feedback for the initial switch of options to a string table. Happy to tweak or adjust if desired here.

Patch is 20.79 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/123308.diff

7 Files Affected:

(modified) clang/lib/Frontend/CompilerInvocation.cpp (+1-1)
(modified) lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp (+2-1)
(modified) llvm/include/llvm/Option/OptTable.h (+38-29)
(modified) llvm/lib/Option/OptTable.cpp (+36-34)
(modified) llvm/tools/llvm-objdump/llvm-objdump.cpp (+2-1)
(modified) llvm/unittests/Option/OptionMarshallingTest.cpp (+2-1)
(modified) llvm/utils/TableGen/OptionParserEmitter.cpp (+7-4)

diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 58658dedbaf1ee..3bf124e4827be9 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -282,7 +282,7 @@ using ArgumentConsumer = CompilerInvocation::ArgumentConsumer;
 #undef OPTTABLE_STR_TABLE_CODE
 
 static llvm::StringRef lookupStrInTable(unsigned Offset) {
-  return &OptionStrTable[Offset];
+  return OptionStrTable[Offset];
 }
 
 #define SIMPLE_ENUM_VALUE_TABLE
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
index 2a36f95c94d0ce..51e9a6d81b8390 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -42,6 +42,7 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Timer.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringTable.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
@@ -1083,7 +1084,7 @@ void PlatformDarwin::AddClangModuleCompilationOptionsForSDKType(
   if (!version.empty() && sdk_type != XcodeSDK::Type::Linux &&
       sdk_type != XcodeSDK::Type::XROS) {
 #define OPTION(PREFIX_OFFSET, NAME_OFFSET, VAR, ...)                           \
-  llvm::StringRef opt_##VAR = &OptionStrTable[NAME_OFFSET];                    \
+  llvm::StringRef opt_##VAR = OptionStrTable[NAME_OFFSET];                     \
   (void)opt_##VAR;
 #include "clang/Driver/Options.inc"
 #undef OPTION
diff --git a/llvm/include/llvm/Option/OptTable.h b/llvm/include/llvm/Option/OptTable.h
index 38a03fef7ae124..61a58aa304ecb4 100644
--- a/llvm/include/llvm/Option/OptTable.h
+++ b/llvm/include/llvm/Option/OptTable.h
@@ -12,6 +12,7 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringTable.h"
 #include "llvm/Option/OptSpecifier.h"
 #include "llvm/Support/StringSaver.h"
 #include <cassert>
@@ -54,7 +55,7 @@ class OptTable {
   /// Entry for a single option instance in the option data table.
   struct Info {
     unsigned PrefixesOffset;
-    unsigned PrefixedNameOffset;
+    StringTable::Offset PrefixedNameOffset;
     const char *HelpText;
     // Help text for specific visibilities. A list of pairs, where each pair
     // is a list of visibilities and a specific help string for those
@@ -80,34 +81,37 @@ class OptTable {
 
     bool hasNoPrefix() const { return PrefixesOffset == 0; }
 
-    unsigned getNumPrefixes(ArrayRef<unsigned> PrefixesTable) const {
-      return PrefixesTable[PrefixesOffset];
+    unsigned getNumPrefixes(ArrayRef<StringTable::Offset> PrefixesTable) const {
+      // We embed the number of prefixes in the value of the first offset.
+      return PrefixesTable[PrefixesOffset].value();
     }
 
-    ArrayRef<unsigned>
-    getPrefixOffsets(ArrayRef<unsigned> PrefixesTable) const {
-      return hasNoPrefix() ? ArrayRef<unsigned>()
+    ArrayRef<StringTable::Offset>
+    getPrefixOffsets(ArrayRef<StringTable::Offset> PrefixesTable) const {
+      return hasNoPrefix() ? ArrayRef<StringTable::Offset>()
                            : PrefixesTable.slice(PrefixesOffset + 1,
                                                  getNumPrefixes(PrefixesTable));
     }
 
-    void appendPrefixes(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+    void appendPrefixes(const StringTable &StrTable,
+                        ArrayRef<StringTable::Offset> PrefixesTable,
                         SmallVectorImpl<StringRef> &Prefixes) const {
-      for (unsigned PrefixOffset : getPrefixOffsets(PrefixesTable))
-        Prefixes.push_back(&StrTable[PrefixOffset]);
+      for (auto PrefixOffset : getPrefixOffsets(PrefixesTable))
+        Prefixes.push_back(StrTable[PrefixOffset]);
     }
 
-    StringRef getPrefix(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+    StringRef getPrefix(const StringTable &StrTable,
+                        ArrayRef<StringTable::Offset> PrefixesTable,
                         unsigned PrefixIndex) const {
-      return &StrTable[getPrefixOffsets(PrefixesTable)[PrefixIndex]];
+      return StrTable[getPrefixOffsets(PrefixesTable)[PrefixIndex]];
     }
 
-    StringRef getPrefixedName(const char *StrTable) const {
-      return &StrTable[PrefixedNameOffset];
+    StringRef getPrefixedName(const StringTable &StrTable) const {
+      return StrTable[PrefixedNameOffset];
     }
 
-    StringRef getName(const char *StrTable,
-                      ArrayRef<unsigned> PrefixesTable) const {
+    StringRef getName(const StringTable &StrTable,
+                      ArrayRef<StringTable::Offset> PrefixesTable) const {
       unsigned PrefixLength =
           hasNoPrefix() ? 0 : getPrefix(StrTable, PrefixesTable, 0).size();
       return getPrefixedName(StrTable).drop_front(PrefixLength);
@@ -117,13 +121,13 @@ class OptTable {
 private:
   // A unified string table for these options. Individual strings are stored as
   // null terminated C-strings at offsets within this table.
-  const char *StrTable;
+  const StringTable *StrTable;
 
   // A table of different sets of prefixes. Each set starts with the number of
   // prefixes in that set followed by that many offsets into the string table
   // for each of the prefix strings. This is essentially a Pascal-string style
   // encoding.
-  ArrayRef<unsigned> PrefixesTable;
+  ArrayRef<StringTable::Offset> PrefixesTable;
 
   /// The option information table.
   ArrayRef<Info> OptionInfos;
@@ -161,7 +165,8 @@ class OptTable {
 protected:
   /// Initialize OptTable using Tablegen'ed OptionInfos. Child class must
   /// manually call \c buildPrefixChars once they are fully constructed.
-  OptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  OptTable(const StringTable &StrTable,
+           ArrayRef<StringTable::Offset> PrefixesTable,
            ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
 
   /// Build (or rebuild) the PrefixChars member.
@@ -171,10 +176,12 @@ class OptTable {
   virtual ~OptTable();
 
   /// Return the string table used for option names.
-  const char *getStrTable() const { return StrTable; }
+  const StringTable &getStrTable() const { return *StrTable; }
 
   /// Return the prefixes table used for option names.
-  ArrayRef<unsigned> getPrefixesTable() const { return PrefixesTable; }
+  ArrayRef<StringTable::Offset> getPrefixesTable() const {
+    return PrefixesTable;
+  }
 
   /// Return the total number of option classes.
   unsigned getNumOptions() const { return OptionInfos.size(); }
@@ -187,25 +194,25 @@ class OptTable {
 
   /// Lookup the name of the given option.
   StringRef getOptionName(OptSpecifier id) const {
-    return getInfo(id).getName(StrTable, PrefixesTable);
+    return getInfo(id).getName(*StrTable, PrefixesTable);
   }
 
   /// Lookup the prefix of the given option.
   StringRef getOptionPrefix(OptSpecifier id) const {
     const Info &I = getInfo(id);
     return I.hasNoPrefix() ? StringRef()
-                           : I.getPrefix(StrTable, PrefixesTable, 0);
+                           : I.getPrefix(*StrTable, PrefixesTable, 0);
   }
 
   void appendOptionPrefixes(OptSpecifier id,
                             SmallVectorImpl<StringRef> &Prefixes) const {
     const Info &I = getInfo(id);
-    I.appendPrefixes(StrTable, PrefixesTable, Prefixes);
+    I.appendPrefixes(*StrTable, PrefixesTable, Prefixes);
   }
 
   /// Lookup the prefixed name of the given option.
   StringRef getOptionPrefixedName(OptSpecifier id) const {
-    return getInfo(id).getPrefixedName(StrTable);
+    return getInfo(id).getPrefixedName(*StrTable);
   }
 
   /// Get the kind of the given option.
@@ -418,19 +425,21 @@ class OptTable {
 /// Specialization of OptTable
 class GenericOptTable : public OptTable {
 protected:
-  GenericOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  GenericOptTable(const StringTable &StrTable,
+                  ArrayRef<StringTable::Offset> PrefixesTable,
                   ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
 };
 
 class PrecomputedOptTable : public OptTable {
 protected:
-  PrecomputedOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  PrecomputedOptTable(const StringTable &StrTable,
+                      ArrayRef<StringTable::Offset> PrefixesTable,
                       ArrayRef<Info> OptionInfos,
-                      ArrayRef<unsigned> PrefixesUnionOffsets,
+                      ArrayRef<StringTable::Offset> PrefixesUnionOffsets,
                       bool IgnoreCase = false)
       : OptTable(StrTable, PrefixesTable, OptionInfos, IgnoreCase) {
-    for (unsigned PrefixOffset : PrefixesUnionOffsets)
-      PrefixesUnion.push_back(&StrTable[PrefixOffset]);
+    for (auto PrefixOffset : PrefixesUnionOffsets)
+      PrefixesUnion.push_back(StrTable[PrefixOffset]);
     buildPrefixChars();
   }
 };
diff --git a/llvm/lib/Option/OptTable.cpp b/llvm/lib/Option/OptTable.cpp
index 87e6f1f12364c2..6d10e6154147ec 100644
--- a/llvm/lib/Option/OptTable.cpp
+++ b/llvm/lib/Option/OptTable.cpp
@@ -33,11 +33,12 @@ using namespace llvm::opt;
 
 namespace {
 struct OptNameLess {
-  const char *StrTable;
-  ArrayRef<unsigned> PrefixesTable;
+  const StringTable *StrTable;
+  ArrayRef<StringTable::Offset> PrefixesTable;
 
-  explicit OptNameLess(const char *StrTable, ArrayRef<unsigned> PrefixesTable)
-      : StrTable(StrTable), PrefixesTable(PrefixesTable) {}
+  explicit OptNameLess(const StringTable &StrTable,
+                       ArrayRef<StringTable::Offset> PrefixesTable)
+      : StrTable(&StrTable), PrefixesTable(PrefixesTable) {}
 
 #ifndef NDEBUG
   inline bool operator()(const OptTable::Info &A,
@@ -45,13 +46,13 @@ struct OptNameLess {
     if (&A == &B)
       return false;
 
-    if (int Cmp = StrCmpOptionName(A.getName(StrTable, PrefixesTable),
-                                   B.getName(StrTable, PrefixesTable)))
+    if (int Cmp = StrCmpOptionName(A.getName(*StrTable, PrefixesTable),
+                                   B.getName(*StrTable, PrefixesTable)))
       return Cmp < 0;
 
     SmallVector<StringRef, 8> APrefixes, BPrefixes;
-    A.appendPrefixes(StrTable, PrefixesTable, APrefixes);
-    B.appendPrefixes(StrTable, PrefixesTable, BPrefixes);
+    A.appendPrefixes(*StrTable, PrefixesTable, APrefixes);
+    B.appendPrefixes(*StrTable, PrefixesTable, BPrefixes);
 
     if (int Cmp = StrCmpOptionPrefixes(APrefixes, BPrefixes))
       return Cmp < 0;
@@ -68,7 +69,7 @@ struct OptNameLess {
   // Support lower_bound between info and an option name.
   inline bool operator()(const OptTable::Info &I, StringRef Name) const {
     // Do not fallback to case sensitive comparison.
-    return StrCmpOptionName(I.getName(StrTable, PrefixesTable), Name, false) <
+    return StrCmpOptionName(I.getName(*StrTable, PrefixesTable), Name, false) <
            0;
   }
 };
@@ -76,9 +77,10 @@ struct OptNameLess {
 
 OptSpecifier::OptSpecifier(const Option *Opt) : ID(Opt->getID()) {}
 
-OptTable::OptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+OptTable::OptTable(const StringTable &StrTable,
+                   ArrayRef<StringTable::Offset> PrefixesTable,
                    ArrayRef<Info> OptionInfos, bool IgnoreCase)
-    : StrTable(StrTable), PrefixesTable(PrefixesTable),
+    : StrTable(&StrTable), PrefixesTable(PrefixesTable),
       OptionInfos(OptionInfos), IgnoreCase(IgnoreCase) {
   // Explicitly zero initialize the error to work around a bug in array
   // value-initialization on MinGW with gcc 4.3.5.
@@ -151,13 +153,13 @@ static bool isInput(const ArrayRef<StringRef> &Prefixes, StringRef Arg) {
 }
 
 /// \returns Matched size. 0 means no match.
-static unsigned matchOption(const char *StrTable,
-                            ArrayRef<unsigned> PrefixesTable,
+static unsigned matchOption(const StringTable &StrTable,
+                            ArrayRef<StringTable::Offset> PrefixesTable,
                             const OptTable::Info *I, StringRef Str,
                             bool IgnoreCase) {
   StringRef Name = I->getName(StrTable, PrefixesTable);
-  for (unsigned PrefixOffset : I->getPrefixOffsets(PrefixesTable)) {
-    StringRef Prefix = &StrTable[PrefixOffset];
+  for (auto PrefixOffset : I->getPrefixOffsets(PrefixesTable)) {
+    StringRef Prefix = StrTable[PrefixOffset];
     if (Str.starts_with(Prefix)) {
       StringRef Rest = Str.substr(Prefix.size());
       bool Matched = IgnoreCase ? Rest.starts_with_insensitive(Name)
@@ -170,13 +172,13 @@ static unsigned matchOption(const char *StrTable,
 }
 
 // Returns true if one of the Prefixes + In.Names matches Option
-static bool optionMatches(const char *StrTable,
-                          ArrayRef<unsigned> PrefixesTable,
+static bool optionMatches(const StringTable &StrTable,
+                          ArrayRef<StringTable::Offset> PrefixesTable,
                           const OptTable::Info &In, StringRef Option) {
   StringRef Name = In.getName(StrTable, PrefixesTable);
   if (Option.consume_back(Name))
-    for (unsigned PrefixOffset : In.getPrefixOffsets(PrefixesTable))
-      if (Option == &StrTable[PrefixOffset])
+    for (auto PrefixOffset : In.getPrefixOffsets(PrefixesTable))
+      if (Option == StrTable[PrefixOffset])
         return true;
   return false;
 }
@@ -189,7 +191,7 @@ OptTable::suggestValueCompletions(StringRef Option, StringRef Arg) const {
   // Search all options and return possible values.
   for (size_t I = FirstSearchableIndex, E = OptionInfos.size(); I < E; I++) {
     const Info &In = OptionInfos[I];
-    if (!In.Values || !optionMatches(StrTable, PrefixesTable, In, Option))
+    if (!In.Values || !optionMatches(*StrTable, PrefixesTable, In, Option))
       continue;
 
     SmallVector<StringRef, 8> Candidates;
@@ -217,9 +219,9 @@ OptTable::findByPrefix(StringRef Cur, Visibility VisibilityMask,
     if (In.Flags & DisableFlags)
       continue;
 
-    StringRef Name = In.getName(StrTable, PrefixesTable);
-    for (unsigned PrefixOffset : In.getPrefixOffsets(PrefixesTable)) {
-      StringRef Prefix = &StrTable[PrefixOffset];
+    StringRef Name = In.getName(*StrTable, PrefixesTable);
+    for (auto PrefixOffset : In.getPrefixOffsets(PrefixesTable)) {
+      StringRef Prefix = (*StrTable)[PrefixOffset];
       std::string S = (Twine(Prefix) + Name + "\t").str();
       if (In.HelpText)
         S += In.HelpText;
@@ -271,7 +273,7 @@ unsigned OptTable::internalFindNearest(
 
   for (const Info &CandidateInfo :
        ArrayRef<Info>(OptionInfos).drop_front(FirstSearchableIndex)) {
-    StringRef CandidateName = CandidateInfo.getName(StrTable, PrefixesTable);
+    StringRef CandidateName = CandidateInfo.getName(*StrTable, PrefixesTable);
 
     // We can eliminate some option prefix/name pairs as candidates right away:
     // * Ignore option candidates with empty names, such as "--", or names
@@ -304,9 +306,9 @@ unsigned OptTable::internalFindNearest(
     // Consider each possible prefix for each candidate to find the most
     // appropriate one. For example, if a user asks for "--helm", suggest
     // "--help" over "-help".
-    for (unsigned CandidatePrefixOffset :
+    for (auto CandidatePrefixOffset :
          CandidateInfo.getPrefixOffsets(PrefixesTable)) {
-      StringRef CandidatePrefix = &StrTable[CandidatePrefixOffset];
+      StringRef CandidatePrefix = (*StrTable)[CandidatePrefixOffset];
       // If Candidate and NormalizedName have more than 'BestDistance'
       // characters of difference, no need to compute the edit distance, it's
       // going to be greater than BestDistance. Don't bother computing Candidate
@@ -359,14 +361,14 @@ std::unique_ptr<Arg> OptTable::parseOneArgGrouped(InputArgList &Args,
   StringRef Name = Str.ltrim(PrefixChars);
   const Info *Start =
       std::lower_bound(OptionInfos.data() + FirstSearchableIndex, End, Name,
-                       OptNameLess(StrTable, PrefixesTable));
+                       OptNameLess(*StrTable, PrefixesTable));
   const Info *Fallback = nullptr;
   unsigned Prev = Index;
 
   // Search for the option which matches Str.
   for (; Start != End; ++Start) {
     unsigned ArgSize =
-        matchOption(StrTable, PrefixesTable, Start, Str, IgnoreCase);
+        matchOption(*StrTable, PrefixesTable, Start, Str, IgnoreCase);
     if (!ArgSize)
       continue;
 
@@ -449,7 +451,7 @@ std::unique_ptr<Arg> OptTable::internalParseOneArg(
 
   // Search for the first next option which could be a prefix.
   Start =
-      std::lower_bound(Start, End, Name, OptNameLess(StrTable, PrefixesTable));
+      std::lower_bound(Start, End, Name, OptNameLess(*StrTable, PrefixesTable));
 
   // Options are stored in sorted order, with '\0' at the end of the
   // alphabet. Since the only options which can accept a string must
@@ -464,7 +466,7 @@ std::unique_ptr<Arg> OptTable::internalParseOneArg(
     // Scan for first option which is a proper prefix.
     for (; Start != End; ++Start)
       if ((ArgSize =
-               matchOption(StrTable, PrefixesTable, Start, Str, IgnoreCase)))
+               matchOption(*StrTable, PrefixesTable, Start, Str, IgnoreCase)))
         break;
     if (Start == End)
       break;
@@ -787,15 +789,15 @@ void OptTable::internalPrintHelp(
   OS.flush();
 }
 
-GenericOptTable::GenericOptTable(const char *StrTable,
-                                 ArrayRef<unsigned> PrefixesTable,
+GenericOptTable::GenericOptTable(const StringTable &StrTable,
+                                 ArrayRef<StringTable::Offset> PrefixesTable,
                                  ArrayRef<Info> OptionInfos, bool IgnoreCase)
     : OptTable(StrTable, PrefixesTable, OptionInfos, IgnoreCase) {
 
   std::set<StringRef> TmpPrefixesUnion;
   for (auto const &Info : OptionInfos.drop_front(FirstSearchableIndex))
-    for (unsigned PrefixOffset : Info.getPrefixOffsets(PrefixesTable))
-      TmpPrefixesUnion.insert(StringRef(&StrTable[PrefixOffset]));
+    for (auto PrefixOffset : Info.getPrefixOffsets(PrefixesTable))
+      TmpPrefixesUnion.insert(StrTable[PrefixOffset]);
   PrefixesUnion.append(TmpPrefixesUnion.begin(), TmpPrefixesUnion.end());
   buildPrefixChars();
 }
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index 93fed8ee8e6f42..99e0440dce78d5 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -94,7 +94,8 @@ namespace {
 
 class CommonOptTable : public opt::GenericOptTable {
 public:
-  CommonOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
+  CommonOptTable(const StringTable &StrTable,
+                 ArrayRef<StringTable::Offset> PrefixesTable,
                  ArrayRef<Info> OptionInfos, const char *Usage,
                  const char *Description)
       : opt::GenericOptTable(StrTable, PrefixesTable, OptionInfos),
diff --git a/llvm/unittests/Option/OptionMarshallingTest.cpp b/llvm/unittests/Option/OptionMarshallingTest.cpp
index 08c3b019689f8c..005144b91bf7f3 100644
--- a/llvm/unittests/Option/OptionMarshallingTest.cpp
+++ b/llvm/unittests/Option/OptionMarshallingTest.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringTable.h"
 #include "gtest/gtest.h"
 
 #define OPTTABLE_STR_TABLE_CODE
@@ -20,7 +21,7 @@ struct OptionWithMarshallingInfo {
   const char *ImpliedValue;
 
   llvm::StringRef getPrefixedName() const {
-    return &OptionStrTable[PrefixedNameOffset];
+    return OptionStrTable[PrefixedNameOffset];
   }
 };
 
diff --git a/llvm/utils/TableGen/OptionParserEmitter.cpp b/llvm/utils/TableGen/OptionParserEmitter.cpp
index 8b92d252392194..35a452890b0ec7 100644
--- a/llvm/utils/TableGen/OptionParserEmitter.cpp
+++ b/llvm/utils/TableGen/OptionParserEmitter.cpp
@@ -303,15 +303,17 @@ static void emitOptionParser(const RecordKeeper &Records, raw_ostream &OS) {
   OS << "/////////\n";
   OS << "// String table\n\n";
   OS << "#ifdef OPTTABLE_STR_TABLE_CODE\n";
-  Table.EmitStringLiteralDef(OS, "static constexpr char OptionStrTable[]",
-                             /*Indent=*/"");
+  Table.EmitStringLiteralDef(
+      OS, "static constexpr llvm::StringTable OptionStrTable",
+      /*Indent=*/"");
   OS << "#endif // OPTTABLE_STR_TABLE_CODE\n\n";
 
   // ...
[truncated]

chandlerc · 2025-01-22T08:42:22Z

Ping

rnk

Thanks, I just had one question.

llvm/lib/Option/OptTable.cpp

llvm/include/llvm/Option/OptTable.h

chandlerc requested a review from rnk January 17, 2025 09:23

chandlerc requested a review from JDevlieghere as a code owner January 17, 2025 09:23

llvmbot added clang Clang issues not falling into any other category lldb tablegen llvm:binary-utilities labels Jan 17, 2025

This was referenced Jan 20, 2025

[StrTable] Switch intrinsics to StringTable and work around MSVC #123548

Merged

Patch series to reapply #118734 and substantially improve it #120534

Closed

rnk reviewed Jan 22, 2025

View reviewed changes

llvm/lib/Option/OptTable.cpp Show resolved Hide resolved

llvm/include/llvm/Option/OptTable.h Show resolved Hide resolved

rnk approved these changes Jan 22, 2025

View reviewed changes

chandlerc merged commit cd5694e into llvm:main Jan 23, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[StrTable] Switch the option parser to `llvm::StringTable` #123308

[StrTable] Switch the option parser to `llvm::StringTable` #123308

Uh oh!

chandlerc commented Jan 17, 2025

Uh oh!

llvmbot commented Jan 17, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jan 17, 2025

Uh oh!

llvmbot commented Jan 17, 2025

Uh oh!

chandlerc commented Jan 22, 2025

Uh oh!

rnk left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[StrTable] Switch the option parser to llvm::StringTable #123308

[StrTable] Switch the option parser to llvm::StringTable #123308

Uh oh!

Conversation

chandlerc commented Jan 17, 2025

Uh oh!

llvmbot commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 17, 2025

Uh oh!

llvmbot commented Jan 17, 2025

Uh oh!

chandlerc commented Jan 22, 2025

Uh oh!

rnk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[StrTable] Switch the option parser to `llvm::StringTable` #123308

[StrTable] Switch the option parser to `llvm::StringTable` #123308

llvmbot commented Jan 17, 2025 •

edited

Loading