This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Refactor command option enum values (NFC)
ClosedPublic

Authored by JDevlieghere on Jul 13 2022, 4:24 PM.

Details

Summary

Refactor command option enum values so that there's a table that connects the CommandArgumentType enumeration with the corresponding enum values. This has two benefits:

  • We guarantee that two options that use the same argument type have the same accepted values.
  • We can print the enum values and their description in the help output (not part of this patch).

Diff Detail

Event Timeline

JDevlieghere created this revision.Jul 13 2022, 4:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 4:24 PM
JDevlieghere requested review of this revision.Jul 13 2022, 4:24 PM

ach, that's great groundwork. I've always been a little annoyed doing settings set stop-disassembly-display bogusvalue to see the list of enumerated value names, but not annoyed enough to get to the bottom of it.

In case anyone is wondering why I created a new table instead of using g_arguments_data, the answers is that I didn't want to pull that whole table, and the help text callbacks it depends on, into a separate header. But after having discovered two entries that were out of order in this table, I'm starting to reconsider that decision to avoid similar mistakes in the future.

  • Merge two tables

Add static_assert that ensures that the entries in the table have the same order as in the enum.

clayborg added inline comments.Jul 14 2022, 2:04 PM
lldb/source/Commands/CommandObjectBreakpointCommand.cpp
25–54

instead of moving all of these global enum declarations into CommandOptionArgumentTable.h, can we register the enum type in a static Initalize method? Maybe leave this code here and then add a function that would register a given enum type with the command interpreter so they can be displayed?

static void CommandObjectBreakpointCommandAdd::Initialize() {
  CommandInterpreter::RegisterEnumerationOption(g_script_option_enumeration, "<script-language>");
}

Then we can leave these definitions where they live, but register then so they can be seen in the help?

JDevlieghere marked an inline comment as done.Jul 14 2022, 3:03 PM
JDevlieghere added inline comments.
lldb/source/Commands/CommandObjectBreakpointCommand.cpp
25–54

That's a great question.

At a technical level that wouldn't work because of the way the option definitions are currently generated. All these tables and their entries are built at compile time (i.e. they're all constexpr). I didn't dig into why that's necessary, but I'm assuming there is a reason for it. But even if we were to change that, these enums shouldn't belong to a specific command. Multiple commands can share these argument types so in that sense it only makes sense that their values are shared as well. Before this patch, it's totally possible to specify that something takes an argument <foo> and one commands accepts a certain set of enum values while the other accepts a totally different set. By centralizing them, there's a unique mapping between argument type and its values, which is exactly what we need to display them in the help.

clayborg accepted this revision.Jul 14 2022, 3:36 PM
clayborg added inline comments.
lldb/source/Commands/CommandObjectBreakpointCommand.cpp
25–54

Makes sense

This revision is now accepted and ready to land.Jul 14 2022, 3:36 PM
This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 9:37 PM