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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
