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

[Raphael Isemann via Phabricator via lldb-commits]

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.

Agreed.

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).


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65386/new/

https://reviews.llvm.org/D65386