Skip to content

Commit cd5694e

Browse files
chandlercchandlerc
authored
[StrTable] Switch the option parser to llvm::StringTable (#123308)
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.
1 parent 8eb99bb commit cd5694e

File tree

7 files changed

+88
-71
lines changed

7 files changed

+88
-71
lines changed

‎clang/lib/Frontend/CompilerInvocation.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ using ArgumentConsumer = CompilerInvocation::ArgumentConsumer;
282282
#undef OPTTABLE_STR_TABLE_CODE
283283

284284
static llvm::StringRef lookupStrInTable(unsigned Offset) {
285-
return &OptionStrTable[Offset];
285+
return OptionStrTable[Offset];
286286
}
287287

288288
#define SIMPLE_ENUM_VALUE_TABLE

‎lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
#include "lldb/Utility/Status.h"
4343
#include "lldb/Utility/Timer.h"
4444
#include "llvm/ADT/STLExtras.h"
45+
#include "llvm/ADT/StringTable.h"
4546
#include "llvm/Support/Error.h"
4647
#include "llvm/Support/FileSystem.h"
4748
#include "llvm/Support/Threading.h"
@@ -1083,7 +1084,7 @@ void PlatformDarwin::AddClangModuleCompilationOptionsForSDKType(
10831084
if (!version.empty() && sdk_type != XcodeSDK::Type::Linux &&
10841085
sdk_type != XcodeSDK::Type::XROS) {
10851086
#define OPTION(PREFIX_OFFSET, NAME_OFFSET, VAR, ...) \
1086-
llvm::StringRef opt_##VAR = &OptionStrTable[NAME_OFFSET]; \
1087+
llvm::StringRef opt_##VAR = OptionStrTable[NAME_OFFSET]; \
10871088
(void)opt_##VAR;
10881089
#include "clang/Driver/Options.inc"
10891090
#undef OPTION

‎llvm/include/llvm/Option/OptTable.h

Lines changed: 38 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include "llvm/ADT/ArrayRef.h"
1313
#include "llvm/ADT/SmallString.h"
1414
#include "llvm/ADT/StringRef.h"
15+
#include "llvm/ADT/StringTable.h"
1516
#include "llvm/Option/OptSpecifier.h"
1617
#include "llvm/Support/StringSaver.h"
1718
#include <cassert>
@@ -54,7 +55,7 @@ class OptTable {
5455
/// Entry for a single option instance in the option data table.
5556
struct Info {
5657
unsigned PrefixesOffset;
57-
unsigned PrefixedNameOffset;
58+
StringTable::Offset PrefixedNameOffset;
5859
const char *HelpText;
5960
// Help text for specific visibilities. A list of pairs, where each pair
6061
// is a list of visibilities and a specific help string for those
@@ -80,34 +81,37 @@ class OptTable {
8081

8182
bool hasNoPrefix() const { return PrefixesOffset == 0; }
8283

83-
unsigned getNumPrefixes(ArrayRef<unsigned> PrefixesTable) const {
84-
return PrefixesTable[PrefixesOffset];
84+
unsigned getNumPrefixes(ArrayRef<StringTable::Offset> PrefixesTable) const {
85+
// We embed the number of prefixes in the value of the first offset.
86+
return PrefixesTable[PrefixesOffset].value();
8587
}
8688

87-
ArrayRef<unsigned>
88-
getPrefixOffsets(ArrayRef<unsigned> PrefixesTable) const {
89-
return hasNoPrefix() ? ArrayRef<unsigned>()
89+
ArrayRef<StringTable::Offset>
90+
getPrefixOffsets(ArrayRef<StringTable::Offset> PrefixesTable) const {
91+
return hasNoPrefix() ? ArrayRef<StringTable::Offset>()
9092
: PrefixesTable.slice(PrefixesOffset + 1,
9193
getNumPrefixes(PrefixesTable));
9294
}
9395

94-
void appendPrefixes(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
96+
void appendPrefixes(const StringTable &StrTable,
97+
ArrayRef<StringTable::Offset> PrefixesTable,
9598
SmallVectorImpl<StringRef> &Prefixes) const {
96-
for (unsigned PrefixOffset : getPrefixOffsets(PrefixesTable))
97-
Prefixes.push_back(&StrTable[PrefixOffset]);
99+
for (auto PrefixOffset : getPrefixOffsets(PrefixesTable))
100+
Prefixes.push_back(StrTable[PrefixOffset]);
98101
}
99102

100-
StringRef getPrefix(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
103+
StringRef getPrefix(const StringTable &StrTable,
104+
ArrayRef<StringTable::Offset> PrefixesTable,
101105
unsigned PrefixIndex) const {
102-
return &StrTable[getPrefixOffsets(PrefixesTable)[PrefixIndex]];
106+
return StrTable[getPrefixOffsets(PrefixesTable)[PrefixIndex]];
103107
}
104108

105-
StringRef getPrefixedName(const char *StrTable) const {
106-
return &StrTable[PrefixedNameOffset];
109+
StringRef getPrefixedName(const StringTable &StrTable) const {
110+
return StrTable[PrefixedNameOffset];
107111
}
108112

109-
StringRef getName(const char *StrTable,
110-
ArrayRef<unsigned> PrefixesTable) const {
113+
StringRef getName(const StringTable &StrTable,
114+
ArrayRef<StringTable::Offset> PrefixesTable) const {
111115
unsigned PrefixLength =
112116
hasNoPrefix() ? 0 : getPrefix(StrTable, PrefixesTable, 0).size();
113117
return getPrefixedName(StrTable).drop_front(PrefixLength);
@@ -117,13 +121,13 @@ class OptTable {
117121
private:
118122
// A unified string table for these options. Individual strings are stored as
119123
// null terminated C-strings at offsets within this table.
120-
const char *StrTable;
124+
const StringTable *StrTable;
121125

122126
// A table of different sets of prefixes. Each set starts with the number of
123127
// prefixes in that set followed by that many offsets into the string table
124128
// for each of the prefix strings. This is essentially a Pascal-string style
125129
// encoding.
126-
ArrayRef<unsigned> PrefixesTable;
130+
ArrayRef<StringTable::Offset> PrefixesTable;
127131

128132
/// The option information table.
129133
ArrayRef<Info> OptionInfos;
@@ -161,7 +165,8 @@ class OptTable {
161165
protected:
162166
/// Initialize OptTable using Tablegen'ed OptionInfos. Child class must
163167
/// manually call \c buildPrefixChars once they are fully constructed.
164-
OptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
168+
OptTable(const StringTable &StrTable,
169+
ArrayRef<StringTable::Offset> PrefixesTable,
165170
ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
166171

167172
/// Build (or rebuild) the PrefixChars member.
@@ -171,10 +176,12 @@ class OptTable {
171176
virtual ~OptTable();
172177

173178
/// Return the string table used for option names.
174-
const char *getStrTable() const { return StrTable; }
179+
const StringTable &getStrTable() const { return *StrTable; }
175180

176181
/// Return the prefixes table used for option names.
177-
ArrayRef<unsigned> getPrefixesTable() const { return PrefixesTable; }
182+
ArrayRef<StringTable::Offset> getPrefixesTable() const {
183+
return PrefixesTable;
184+
}
178185

179186
/// Return the total number of option classes.
180187
unsigned getNumOptions() const { return OptionInfos.size(); }
@@ -187,25 +194,25 @@ class OptTable {
187194

188195
/// Lookup the name of the given option.
189196
StringRef getOptionName(OptSpecifier id) const {
190-
return getInfo(id).getName(StrTable, PrefixesTable);
197+
return getInfo(id).getName(*StrTable, PrefixesTable);
191198
}
192199

193200
/// Lookup the prefix of the given option.
194201
StringRef getOptionPrefix(OptSpecifier id) const {
195202
const Info &I = getInfo(id);
196203
return I.hasNoPrefix() ? StringRef()
197-
: I.getPrefix(StrTable, PrefixesTable, 0);
204+
: I.getPrefix(*StrTable, PrefixesTable, 0);
198205
}
199206

200207
void appendOptionPrefixes(OptSpecifier id,
201208
SmallVectorImpl<StringRef> &Prefixes) const {
202209
const Info &I = getInfo(id);
203-
I.appendPrefixes(StrTable, PrefixesTable, Prefixes);
210+
I.appendPrefixes(*StrTable, PrefixesTable, Prefixes);
204211
}
205212

206213
/// Lookup the prefixed name of the given option.
207214
StringRef getOptionPrefixedName(OptSpecifier id) const {
208-
return getInfo(id).getPrefixedName(StrTable);
215+
return getInfo(id).getPrefixedName(*StrTable);
209216
}
210217

211218
/// Get the kind of the given option.
@@ -418,19 +425,21 @@ class OptTable {
418425
/// Specialization of OptTable
419426
class GenericOptTable : public OptTable {
420427
protected:
421-
GenericOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
428+
GenericOptTable(const StringTable &StrTable,
429+
ArrayRef<StringTable::Offset> PrefixesTable,
422430
ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
423431
};
424432

425433
class PrecomputedOptTable : public OptTable {
426434
protected:
427-
PrecomputedOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
435+
PrecomputedOptTable(const StringTable &StrTable,
436+
ArrayRef<StringTable::Offset> PrefixesTable,
428437
ArrayRef<Info> OptionInfos,
429-
ArrayRef<unsigned> PrefixesUnionOffsets,
438+
ArrayRef<StringTable::Offset> PrefixesUnionOffsets,
430439
bool IgnoreCase = false)
431440
: OptTable(StrTable, PrefixesTable, OptionInfos, IgnoreCase) {
432-
for (unsigned PrefixOffset : PrefixesUnionOffsets)
433-
PrefixesUnion.push_back(&StrTable[PrefixOffset]);
441+
for (auto PrefixOffset : PrefixesUnionOffsets)
442+
PrefixesUnion.push_back(StrTable[PrefixOffset]);
434443
buildPrefixChars();
435444
}
436445
};

‎llvm/lib/Option/OptTable.cpp

Lines changed: 36 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -33,25 +33,26 @@ using namespace llvm::opt;
3333

3434
namespace {
3535
struct OptNameLess {
36-
const char *StrTable;
37-
ArrayRef<unsigned> PrefixesTable;
36+
const StringTable *StrTable;
37+
ArrayRef<StringTable::Offset> PrefixesTable;
3838

39-
explicit OptNameLess(const char *StrTable, ArrayRef<unsigned> PrefixesTable)
40-
: StrTable(StrTable), PrefixesTable(PrefixesTable) {}
39+
explicit OptNameLess(const StringTable &StrTable,
40+
ArrayRef<StringTable::Offset> PrefixesTable)
41+
: StrTable(&StrTable), PrefixesTable(PrefixesTable) {}
4142

4243
#ifndef NDEBUG
4344
inline bool operator()(const OptTable::Info &A,
4445
const OptTable::Info &B) const {
4546
if (&A == &B)
4647
return false;
4748

48-
if (int Cmp = StrCmpOptionName(A.getName(StrTable, PrefixesTable),
49-
B.getName(StrTable, PrefixesTable)))
49+
if (int Cmp = StrCmpOptionName(A.getName(*StrTable, PrefixesTable),
50+
B.getName(*StrTable, PrefixesTable)))
5051
return Cmp < 0;
5152

5253
SmallVector<StringRef, 8> APrefixes, BPrefixes;
53-
A.appendPrefixes(StrTable, PrefixesTable, APrefixes);
54-
B.appendPrefixes(StrTable, PrefixesTable, BPrefixes);
54+
A.appendPrefixes(*StrTable, PrefixesTable, APrefixes);
55+
B.appendPrefixes(*StrTable, PrefixesTable, BPrefixes);
5556

5657
if (int Cmp = StrCmpOptionPrefixes(APrefixes, BPrefixes))
5758
return Cmp < 0;
@@ -68,17 +69,18 @@ struct OptNameLess {
6869
// Support lower_bound between info and an option name.
6970
inline bool operator()(const OptTable::Info &I, StringRef Name) const {
7071
// Do not fallback to case sensitive comparison.
71-
return StrCmpOptionName(I.getName(StrTable, PrefixesTable), Name, false) <
72+
return StrCmpOptionName(I.getName(*StrTable, PrefixesTable), Name, false) <
7273
0;
7374
}
7475
};
7576
} // namespace
7677

7778
OptSpecifier::OptSpecifier(const Option *Opt) : ID(Opt->getID()) {}
7879

79-
OptTable::OptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
80+
OptTable::OptTable(const StringTable &StrTable,
81+
ArrayRef<StringTable::Offset> PrefixesTable,
8082
ArrayRef<Info> OptionInfos, bool IgnoreCase)
81-
: StrTable(StrTable), PrefixesTable(PrefixesTable),
83+
: StrTable(&StrTable), PrefixesTable(PrefixesTable),
8284
OptionInfos(OptionInfos), IgnoreCase(IgnoreCase) {
8385
// Explicitly zero initialize the error to work around a bug in array
8486
// value-initialization on MinGW with gcc 4.3.5.
@@ -151,13 +153,13 @@ static bool isInput(const ArrayRef<StringRef> &Prefixes, StringRef Arg) {
151153
}
152154

153155
/// \returns Matched size. 0 means no match.
154-
static unsigned matchOption(const char *StrTable,
155-
ArrayRef<unsigned> PrefixesTable,
156+
static unsigned matchOption(const StringTable &StrTable,
157+
ArrayRef<StringTable::Offset> PrefixesTable,
156158
const OptTable::Info *I, StringRef Str,
157159
bool IgnoreCase) {
158160
StringRef Name = I->getName(StrTable, PrefixesTable);
159-
for (unsigned PrefixOffset : I->getPrefixOffsets(PrefixesTable)) {
160-
StringRef Prefix = &StrTable[PrefixOffset];
161+
for (auto PrefixOffset : I->getPrefixOffsets(PrefixesTable)) {
162+
StringRef Prefix = StrTable[PrefixOffset];
161163
if (Str.starts_with(Prefix)) {
162164
StringRef Rest = Str.substr(Prefix.size());
163165
bool Matched = IgnoreCase ? Rest.starts_with_insensitive(Name)
@@ -170,13 +172,13 @@ static unsigned matchOption(const char *StrTable,
170172
}
171173

172174
// Returns true if one of the Prefixes + In.Names matches Option
173-
static bool optionMatches(const char *StrTable,
174-
ArrayRef<unsigned> PrefixesTable,
175+
static bool optionMatches(const StringTable &StrTable,
176+
ArrayRef<StringTable::Offset> PrefixesTable,
175177
const OptTable::Info &In, StringRef Option) {
176178
StringRef Name = In.getName(StrTable, PrefixesTable);
177179
if (Option.consume_back(Name))
178-
for (unsigned PrefixOffset : In.getPrefixOffsets(PrefixesTable))
179-
if (Option == &StrTable[PrefixOffset])
180+
for (auto PrefixOffset : In.getPrefixOffsets(PrefixesTable))
181+
if (Option == StrTable[PrefixOffset])
180182
return true;
181183
return false;
182184
}
@@ -189,7 +191,7 @@ OptTable::suggestValueCompletions(StringRef Option, StringRef Arg) const {
189191
// Search all options and return possible values.
190192
for (size_t I = FirstSearchableIndex, E = OptionInfos.size(); I < E; I++) {
191193
const Info &In = OptionInfos[I];
192-
if (!In.Values || !optionMatches(StrTable, PrefixesTable, In, Option))
194+
if (!In.Values || !optionMatches(*StrTable, PrefixesTable, In, Option))
193195
continue;
194196

195197
SmallVector<StringRef, 8> Candidates;
@@ -217,9 +219,9 @@ OptTable::findByPrefix(StringRef Cur, Visibility VisibilityMask,
217219
if (In.Flags & DisableFlags)
218220
continue;
219221

220-
StringRef Name = In.getName(StrTable, PrefixesTable);
221-
for (unsigned PrefixOffset : In.getPrefixOffsets(PrefixesTable)) {
222-
StringRef Prefix = &StrTable[PrefixOffset];
222+
StringRef Name = In.getName(*StrTable, PrefixesTable);
223+
for (auto PrefixOffset : In.getPrefixOffsets(PrefixesTable)) {
224+
StringRef Prefix = (*StrTable)[PrefixOffset];
223225
std::string S = (Twine(Prefix) + Name + "\t").str();
224226
if (In.HelpText)
225227
S += In.HelpText;
@@ -271,7 +273,7 @@ unsigned OptTable::internalFindNearest(
271273

272274
for (const Info &CandidateInfo :
273275
ArrayRef<Info>(OptionInfos).drop_front(FirstSearchableIndex)) {
274-
StringRef CandidateName = CandidateInfo.getName(StrTable, PrefixesTable);
276+
StringRef CandidateName = CandidateInfo.getName(*StrTable, PrefixesTable);
275277

276278
// We can eliminate some option prefix/name pairs as candidates right away:
277279
// * Ignore option candidates with empty names, such as "--", or names
@@ -304,9 +306,9 @@ unsigned OptTable::internalFindNearest(
304306
// Consider each possible prefix for each candidate to find the most
305307
// appropriate one. For example, if a user asks for "--helm", suggest
306308
// "--help" over "-help".
307-
for (unsigned CandidatePrefixOffset :
309+
for (auto CandidatePrefixOffset :
308310
CandidateInfo.getPrefixOffsets(PrefixesTable)) {
309-
StringRef CandidatePrefix = &StrTable[CandidatePrefixOffset];
311+
StringRef CandidatePrefix = (*StrTable)[CandidatePrefixOffset];
310312
// If Candidate and NormalizedName have more than 'BestDistance'
311313
// characters of difference, no need to compute the edit distance, it's
312314
// going to be greater than BestDistance. Don't bother computing Candidate
@@ -359,14 +361,14 @@ std::unique_ptr<Arg> OptTable::parseOneArgGrouped(InputArgList &Args,
359361
StringRef Name = Str.ltrim(PrefixChars);
360362
const Info *Start =
361363
std::lower_bound(OptionInfos.data() + FirstSearchableIndex, End, Name,
362-
OptNameLess(StrTable, PrefixesTable));
364+
OptNameLess(*StrTable, PrefixesTable));
363365
const Info *Fallback = nullptr;
364366
unsigned Prev = Index;
365367

366368
// Search for the option which matches Str.
367369
for (; Start != End; ++Start) {
368370
unsigned ArgSize =
369-
matchOption(StrTable, PrefixesTable, Start, Str, IgnoreCase);
371+
matchOption(*StrTable, PrefixesTable, Start, Str, IgnoreCase);
370372
if (!ArgSize)
371373
continue;
372374

@@ -449,7 +451,7 @@ std::unique_ptr<Arg> OptTable::internalParseOneArg(
449451

450452
// Search for the first next option which could be a prefix.
451453
Start =
452-
std::lower_bound(Start, End, Name, OptNameLess(StrTable, PrefixesTable));
454+
std::lower_bound(Start, End, Name, OptNameLess(*StrTable, PrefixesTable));
453455

454456
// Options are stored in sorted order, with '\0' at the end of the
455457
// alphabet. Since the only options which can accept a string must
@@ -464,7 +466,7 @@ std::unique_ptr<Arg> OptTable::internalParseOneArg(
464466
// Scan for first option which is a proper prefix.
465467
for (; Start != End; ++Start)
466468
if ((ArgSize =
467-
matchOption(StrTable, PrefixesTable, Start, Str, IgnoreCase)))
469+
matchOption(*StrTable, PrefixesTable, Start, Str, IgnoreCase)))
468470
break;
469471
if (Start == End)
470472
break;
@@ -787,15 +789,15 @@ void OptTable::internalPrintHelp(
787789
OS.flush();
788790
}
789791

790-
GenericOptTable::GenericOptTable(const char *StrTable,
791-
ArrayRef<unsigned> PrefixesTable,
792+
GenericOptTable::GenericOptTable(const StringTable &StrTable,
793+
ArrayRef<StringTable::Offset> PrefixesTable,
792794
ArrayRef<Info> OptionInfos, bool IgnoreCase)
793795
: OptTable(StrTable, PrefixesTable, OptionInfos, IgnoreCase) {
794796

795797
std::set<StringRef> TmpPrefixesUnion;
796798
for (auto const &Info : OptionInfos.drop_front(FirstSearchableIndex))
797-
for (unsigned PrefixOffset : Info.getPrefixOffsets(PrefixesTable))
798-
TmpPrefixesUnion.insert(StringRef(&StrTable[PrefixOffset]));
799+
for (auto PrefixOffset : Info.getPrefixOffsets(PrefixesTable))
800+
TmpPrefixesUnion.insert(StrTable[PrefixOffset]);
799801
PrefixesUnion.append(TmpPrefixesUnion.begin(), TmpPrefixesUnion.end());
800802
buildPrefixChars();
801803
}

‎llvm/tools/llvm-objdump/llvm-objdump.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,8 @@ namespace {
9494

9595
class CommonOptTable : public opt::GenericOptTable {
9696
public:
97-
CommonOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
97+
CommonOptTable(const StringTable &StrTable,
98+
ArrayRef<StringTable::Offset> PrefixesTable,
9899
ArrayRef<Info> OptionInfos, const char *Usage,
99100
const char *Description)
100101
: opt::GenericOptTable(StrTable, PrefixesTable, OptionInfos),

0 commit comments

Comments
 (0)