zturner created this revision.
zturner added reviewers: clayborg, labath.
zturner added a subscriber: lldb-commits.
The major blocker to this until now has been that we can't have global
constructors, no matter how trivial.
Recently LLVM obtained the `StringLiteral` class which addresses this. This
class has a `constexpr` constructor, so as long as a global `StringRef` (or
class/struct containing a `StringRef` is declared `constexpr`, the `StringRef`
can be initialized with a `StringLiteral` and not invoke a global constructor.
This probably sounds confusing, but in practice it's simple. It looks like
this:
// ok
constexpr StringRef S = llvm::StringLiteral("foo");
// ok
constexpr StringLiteral T("foo");
// not ok, global constructor invoked.
StringLiteral U("foo");
struct Foo {
constexpr Foo(StringRef S) : S(S) {}
StringRef S;
};
// ok
constexpr Foo F(llvm::StringLiteral("foo"));
Admittedly, it's pretty verbose to type out `StringLiteral` everywhere. To get
around this, I alias it to `L` in the individual TUs where we use it. This way
we can write:
using L = llvm::StringLiteral;
static constexpr OptionDefinition[] g_defs = { ..., ..., L("opt"), ..., ...,
L("help text") };
Again, note that no global constructor is invoked here, even though this is an
array.
The advantage of doing all of this is that we can now have access to all the
wonderful member functions of `StringRef`. Upon fixing up all the compilation
failures (there were very few however) triggered by the change, we can already
see some code being simplified, and in the future I would like to extend this
to other classes like `RegisterInfo` etc as well.
Note that this patch is incomplete as is. Before submitting, I would need to
go change EVERY global definition of `OptionDefinition` to be marked
`constexpr`, otherwise a global constructor would be invoked. I intentionally
did it in only 2 places here, just to illustrate the idea, before changing the
code everywhere.
https://reviews.llvm.org/D27780
Files:
lldb/include/lldb/Interpreter/Options.h
lldb/include/lldb/lldb-private-types.h
lldb/source/Commands/CommandObjectSettings.cpp
lldb/source/Host/common/OptionParser.cpp
lldb/source/Interpreter/Args.cpp
lldb/source/Interpreter/OptionGroupOutputFile.cpp
lldb/source/Interpreter/Options.cpp
Index: lldb/source/Interpreter/Options.cpp
===================================================================
--- lldb/source/Interpreter/Options.cpp
+++ lldb/source/Interpreter/Options.cpp
@@ -333,27 +333,14 @@
}
}
-bool Options::SupportsLongOption(const char *long_option) {
- if (!long_option || !long_option[0])
+bool Options::SupportsLongOption(llvm::StringRef long_option) {
+ long_option.consume_front("--");
+ if (long_option.empty())
return false;
- auto opt_defs = GetDefinitions();
- if (opt_defs.empty())
- return false;
-
- const char *long_option_name = long_option;
- if (long_option[0] == '-' && long_option[1] == '-')
- long_option_name += 2;
-
- for (auto &def : opt_defs) {
- if (!def.long_option)
- continue;
-
- if (strcmp(def.long_option, long_option_name) == 0)
- return true;
- }
-
- return false;
+ return llvm::any_of(GetDefinitions(), [=](const OptionDefinition &D) {
+ return D.long_option == long_option;
+ });
}
enum OptionDisplayType {
@@ -603,9 +590,10 @@
strm.IndentMore(5);
- if (opt_defs[i].usage_text)
+ if (!opt_defs[i].usage_text.empty())
OutputFormattedUsageText(strm, opt_defs[i], screen_width);
- if (opt_defs[i].enum_values != nullptr) {
+
+ if (opt_defs[i].enum_values == nullptr) {
strm.Indent();
strm.Printf("Values: ");
for (int k = 0; opt_defs[i].enum_values[k].string_value != nullptr;
@@ -670,10 +658,6 @@
auto opt_defs = GetDefinitions();
- std::string cur_opt_std_str(input.GetArgumentAtIndex(cursor_index));
- cur_opt_std_str.erase(char_pos);
- const char *cur_opt_str = cur_opt_std_str.c_str();
-
for (size_t i = 0; i < opt_element_vector.size(); i++) {
int opt_pos = opt_element_vector[i].opt_pos;
int opt_arg_pos = opt_element_vector[i].opt_arg_pos;
@@ -708,18 +692,17 @@
}
return true;
} else if (opt_defs_index != OptionArgElement::eUnrecognizedArg) {
+ auto cur_opt_str = input[cursor_index].ref.take_front(char_pos);
+
// We recognized it, if it an incomplete long option, complete it anyway
- // (getopt_long_only is
- // happy with shortest unique string, but it's still a nice thing to
- // do.) Otherwise return
- // The string so the upper level code will know this is a full match and
- // add the " ".
- if (cur_opt_str && strlen(cur_opt_str) > 2 && cur_opt_str[0] == '-' &&
- cur_opt_str[1] == '-' &&
- strcmp(opt_defs[opt_defs_index].long_option, cur_opt_str) != 0) {
+ // (getopt_long_only is happy with shortest unique string, but it's
+ // still a nice thing to do.) Otherwise return the string so the upper
+ // level code will know this is a full match and add the " ".
+ if (cur_opt_str.startswith("--") &&
+ cur_opt_str.drop_front(2) != opt_defs[opt_defs_index].long_option) {
std::string full_name("--");
full_name.append(opt_defs[opt_defs_index].long_option);
- matches.AppendString(full_name.c_str());
+ matches.AppendString(full_name);
return true;
} else {
matches.AppendString(input.GetArgumentAtIndex(cursor_index));
@@ -729,33 +712,32 @@
// FIXME - not handling wrong options yet:
// Check to see if they are writing a long option & complete it.
// I think we will only get in here if the long option table has two
- // elements
- // that are not unique up to this point. getopt_long_only does shortest
- // unique match
- // for long options already.
-
- if (cur_opt_str && strlen(cur_opt_str) > 2 && cur_opt_str[0] == '-' &&
- cur_opt_str[1] == '-') {
- for (auto &def : opt_defs) {
- if (!def.long_option)
- continue;
+ // elements that are not unique up to this point. getopt_long_only
+ // does shortest unique match for long options already.
+ auto cur_opt_str = input[cursor_index].ref.take_front(char_pos);
- if (strstr(def.long_option, cur_opt_str + 2) == def.long_option) {
- std::string full_name("--");
- full_name.append(def.long_option);
- // The options definitions table has duplicates because of the
- // way the grouping information is stored, so only add once.
- bool duplicate = false;
- for (size_t k = 0; k < matches.GetSize(); k++) {
- if (matches.GetStringAtIndex(k) == full_name) {
- duplicate = true;
- break;
- }
- }
- if (!duplicate)
- matches.AppendString(full_name.c_str());
+ if (!cur_opt_str.consume_front("--"))
+ return true;
+
+ auto range = llvm::make_filter_range(
+ opt_defs, [cur_opt_str](const OptionDefinition &D) {
+ return D.long_option == cur_opt_str;
+ });
+
+ for (auto &def : range) {
+ std::string full_name("--");
+ full_name.append(def.long_option);
+ // The options definitions table has duplicates because of the way the
+ // grouping information is stored, so only add once.
+ bool duplicate = false;
+ for (size_t k = 0; k < matches.GetSize(); k++) {
+ if (matches.GetStringAtIndex(k) == full_name) {
+ duplicate = true;
+ break;
}
}
+ if (!duplicate)
+ matches.AppendString(full_name);
}
return true;
}
@@ -845,14 +827,13 @@
continue;
int cur_arg_pos = opt_element_vector[i].opt_arg_pos;
- const char *cur_opt_name = opt_defs[cur_defs_index].long_option;
+ auto cur_opt_name = opt_defs[cur_defs_index].long_option;
// If this is the "shlib" option and there was an argument provided,
// restrict it to that shared library.
- if (cur_opt_name && strcmp(cur_opt_name, "shlib") == 0 &&
- cur_arg_pos != -1) {
- const char *module_name = input.GetArgumentAtIndex(cur_arg_pos);
- if (module_name) {
+ if (cur_opt_name == "shlib" && cur_arg_pos != -1) {
+ auto module_name = input[cur_arg_pos].ref;
+ if (!module_name.empty()) {
FileSpec module_spec(module_name, false);
lldb::TargetSP target_sp =
interpreter.GetDebugger().GetSelectedTarget();
Index: lldb/source/Interpreter/OptionGroupOutputFile.cpp
===================================================================
--- lldb/source/Interpreter/OptionGroupOutputFile.cpp
+++ lldb/source/Interpreter/OptionGroupOutputFile.cpp
@@ -23,15 +23,17 @@
OptionGroupOutputFile::~OptionGroupOutputFile() {}
-static const uint32_t SHORT_OPTION_APND = 0x61706e64; // 'apnd'
+static const int SHORT_OPTION_APND = 0x61706e64; // 'apnd'
-static OptionDefinition g_option_table[] = {
- {LLDB_OPT_SET_1, false, "outfile", 'o', OptionParser::eRequiredArgument,
+using L = llvm::StringLiteral;
+
+static constexpr OptionDefinition g_option_table[] = {
+ {LLDB_OPT_SET_1, false, L("outfile"), 'o', OptionParser::eRequiredArgument,
nullptr, nullptr, 0, eArgTypeFilename,
- "Specify a path for capturing command output."},
- {LLDB_OPT_SET_1, false, "append-outfile", SHORT_OPTION_APND,
+ L("Specify a path for capturing command output.")},
+ {LLDB_OPT_SET_1, false, L("append-outfile"), SHORT_OPTION_APND,
OptionParser::eNoArgument, nullptr, nullptr, 0, eArgTypeNone,
- "Append to the file specified with '--outfile <path>'."},
+ L("Append to the file specified with '--outfile <path>'.")},
};
llvm::ArrayRef<OptionDefinition> OptionGroupOutputFile::GetDefinitions() {
Index: lldb/source/Interpreter/Args.cpp
===================================================================
--- lldb/source/Interpreter/Args.cpp
+++ lldb/source/Interpreter/Args.cpp
@@ -26,9 +26,12 @@
#include "lldb/Target/Target.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/FormatVariadic.h"
+
using namespace lldb;
using namespace lldb_private;
@@ -947,12 +950,10 @@
size_t Args::FindArgumentIndexForOption(Option *long_options,
int long_options_index) const {
- char short_buffer[3];
- char long_buffer[255];
- ::snprintf(short_buffer, sizeof(short_buffer), "-%c",
- long_options[long_options_index].val);
- ::snprintf(long_buffer, sizeof(long_buffer), "--%s",
- long_options[long_options_index].definition->long_option);
+ llvm::SmallString<3> short_buffer =
+ llvm::formatv("-{0}", (char)long_options[long_options_index].val);
+ llvm::SmallString<255> long_buffer = llvm::formatv(
+ "--{0}", long_options[long_options_index].definition->long_option);
for (auto entry : llvm::enumerate(m_entries)) {
if (entry.Value.ref.startswith(short_buffer) ||
Index: lldb/source/Host/common/OptionParser.cpp
===================================================================
--- lldb/source/Host/common/OptionParser.cpp
+++ lldb/source/Host/common/OptionParser.cpp
@@ -11,6 +11,9 @@
#include "lldb/Host/HostGetOpt.h"
#include "lldb/lldb-private-types.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/StringSaver.h"
+
#include <vector>
using namespace lldb_private;
@@ -31,11 +34,14 @@
int OptionParser::Parse(int argc, char *const argv[], llvm::StringRef optstring,
const Option *longopts, int *longindex) {
std::vector<option> opts;
+ llvm::BumpPtrAllocator allocator;
+ llvm::StringSaver saver(allocator);
+
while (longopts->definition != nullptr) {
option opt;
opt.flag = longopts->flag;
opt.val = longopts->val;
- opt.name = longopts->definition->long_option;
+ opt.name = saver.save(longopts->definition->long_option).data();
opt.has_arg = longopts->definition->option_has_arg;
opts.push_back(opt);
++longopts;
Index: lldb/source/Commands/CommandObjectSettings.cpp
===================================================================
--- lldb/source/Commands/CommandObjectSettings.cpp
+++ lldb/source/Commands/CommandObjectSettings.cpp
@@ -27,9 +27,11 @@
// CommandObjectSettingsSet
//-------------------------------------------------------------------------
-static OptionDefinition g_settings_set_options[] = {
+using L = llvm::StringLiteral;
+
+static constexpr OptionDefinition g_settings_set_options[] = {
// clang-format off
- { LLDB_OPT_SET_2, false, "global", 'g', OptionParser::eNoArgument, nullptr, nullptr, 0, eArgTypeNone, "Apply the new value to the global default value." }
+ { LLDB_OPT_SET_2, false, L("global"), 'g', OptionParser::eNoArgument, nullptr, nullptr, 0, eArgTypeNone, L("Apply the new value to the global default value.") }
// clang-format on
};
Index: lldb/include/lldb/lldb-private-types.h
===================================================================
--- lldb/include/lldb/lldb-private-types.h
+++ lldb/include/lldb/lldb-private-types.h
@@ -104,22 +104,51 @@
};
struct OptionDefinition {
- uint32_t usage_mask; // Used to mark options that can be used together. If (1
- // << n & usage_mask) != 0
- // then this option belongs to option set n.
- bool required; // This option is required (in the current usage level)
- const char *long_option; // Full name for this option.
- int short_option; // Single character for this option.
- int option_has_arg; // no_argument, required_argument or optional_argument
- OptionValidator *validator; // If non-NULL, option is valid iff
- // |validator->IsValid()|, otherwise always valid.
- OptionEnumValueElement *enum_values; // If non-NULL an array of enum values.
- uint32_t completion_type; // Cookie the option class can use to do define the
- // argument completion.
- lldb::CommandArgumentType argument_type; // Type of argument this option takes
- const char *usage_text; // Full text explaining what this options does and
- // what (if any) argument to
- // pass it.
+ OptionDefinition() {}
+
+ constexpr OptionDefinition(uint32_t mask, bool required,
+ llvm::StringRef long_opt, int short_opt,
+ int has_arg, OptionValidator *validator,
+ OptionEnumValueElement *enum_values,
+ uint32_t completion_type,
+ lldb::CommandArgumentType arg_type,
+ llvm::StringRef usage)
+ : usage_mask(mask), required(required), long_option(long_opt),
+ short_option(short_opt), option_has_arg(has_arg), validator(validator),
+ enum_values(enum_values), completion_type(completion_type),
+ argument_type(arg_type), usage_text(usage_text) {}
+
+ // Used to mark options that can be used together. If
+ // (1 << n & usage_mask) != 0 then this option belongs to option set n.
+ uint32_t usage_mask = 0;
+
+ // This option is required (in the current usage level)
+ bool required = false;
+
+ // Full name for this option.
+ llvm::StringRef long_option;
+
+ // Single character for this option.
+ int short_option = 0;
+
+ // no_argument, required_argument or optional_argument
+ int option_has_arg = 0;
+
+ // If non-NULL, option is valid iff |validator->IsValid()|, otherwise always
+ // valid.
+ OptionValidator *validator = nullptr;
+
+ // If non-NULL an array of enum values.
+ OptionEnumValueElement *enum_values = nullptr;
+
+ // Cookie the option class can use to do define the argument completion.
+ uint32_t completion_type = 0;
+
+ // Type of argument this option takes
+ lldb::CommandArgumentType argument_type = lldb::eArgTypeNone;
+
+ // Full text explaining what this options does and what argument(s) it takes.
+ llvm::StringRef usage_text;
};
typedef struct type128 { uint64_t x[2]; } type128;
Index: lldb/include/lldb/Interpreter/Options.h
===================================================================
--- lldb/include/lldb/Interpreter/Options.h
+++ lldb/include/lldb/Interpreter/Options.h
@@ -155,7 +155,7 @@
void GenerateOptionUsage(Stream &strm, CommandObject *cmd,
uint32_t screen_width);
- bool SupportsLongOption(const char *long_option);
+ bool SupportsLongOption(llvm::StringRef long_option);
// The following two pure virtual functions must be defined by every
// class that inherits from this class.
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
