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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D65386#1609875 <https://reviews.llvm.org/D65386#1609875>, @teemperor wrote:

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


If they would really be just one-liners, then this might still be an improvement over the current solution, because now you need at least three lines for each option:

  case eFoo:
    foo = bar;
    break;

And this doesn't include the boilerplate around the switch statement -- i.e., checking the result of `toOptionEnumSettingsSet -- it's not even clear to me under which circumstances can `toOptionEnumSettingsSet` fail to return a value. Shouldn't the caller verify that it is calling us with the correct argument? If we're able to avoid that and just have the user code be:

  switch(toOptionEnumSettingsSet(???)) {
  ...
  }

then I would find the enum solution fine. However, right now, it seems more complicated than it ought to be..

Additionally, if setting the option is really that simple, then we could have tablegen generate that too (by just giving it a variable name to set), possibly with the option to fall back to a function for more complex options (which is better handled in a separate function anyway).

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

I agree, but on the other hand, temporary workarounds have a habit of becoming permanent, so I'd like to avoid introducing a sub-optimal solution, if there's a better way to do that available..


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D65386