clayborg accepted this revision.
clayborg added inline comments.
This revision is now accepted and ready to land.
================
Comment at: lldb/source/Commands/CommandObjectBreakpointCommand.cpp:25-54
-// FIXME: "script-type" needs to have its contents determined dynamically, so
-// somebody can add a new scripting language to lldb and have it pickable here
-// without having to change this enumeration by hand and rebuild lldb proper.
-static constexpr OptionEnumValueElement g_script_option_enumeration[] = {
- {
- eScriptLanguageNone,
- "command",
----------------
JDevlieghere wrote:
> clayborg wrote:
> > 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?
> 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.
Makes sense
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129703/new/
https://reviews.llvm.org/D129703
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
