[Lldb-commits] [PATCH] D65386: [lldb][NFC] Use an enum instead of chars when handling options [WIP]

Raphael Isemann via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Aug 1 02:34:15 PDT 2019

teemperor marked an inline comment as done.
teemperor added a comment.

> It worries me a little bit that we are making it harder and harder to figure out "where does the option for "-t" get stored once this CommandObject's options have been parsed. Can you show the steps I would have to go through to get from "-f" to OptionEnumSettingsSet::Force or whatever.

That's actually just `toOptionEnumSettingsSet('-f', error)`. I want to get rid of the whole generated method by just placing the enum value in the OptionDefinition struct (which requires some refactoring, but should be doable in the long-term).

> The thing I don't like about the enum approach is that it adds another layer to the option-setting code, whereas I think that the main problem is that the option-setting code has one too many layers already.


In D65386#1604498 <https://reviews.llvm.org/D65386#1604498>, @labath wrote:

> How about codegenning the entire implementation of `SetOptionValue`? That way the user won't have to write any switch statements at all. Ideally, the option-setting code would be something like:
>   void (Status?, Error?) SetOptionForce(StringRef arg, ExecutionContext *ctx) { m_force = true; }
>   void (Status?, Error?) SetOptionGlobal(StringRef arg, ExecutionContext *ctx) { m_global = true; }
>   #include "The_thing_which_generates_SetOptionValue.inc"
> The generated implementation of SetOptionValue could be the same as the current one, except that it calls into these user-specified functions instead of setting the values itself

This seems like a lot of boilerplate when we have to write 300+ one-statement methods for assigning options. Also I would prefer to not use tablegen for generating executable code if possible because that is just hard to read (the function we generate here is already something I only consider as a temporary workaround).

Comment at: lldb/utils/TableGen/LLDBOptionDefEmitter.cpp:211
+  std::string CamelCaseID = ToCamelCase(Command);
JDevlieghere wrote:
> Can we use the option name instead, like I did for the properties? Or would that cause conflicts? 
If you mean if we can just call it `OptionEnumSet` instead of `OptionEnumSettingsSet`, then I assume that could cause conflicts if we implement multiple smaller commands in the same file (which we currently do).




More information about the lldb-commits mailing list