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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Aug 1 04:27:02 PDT 2019


labath added a comment.

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

> In D65386#1609956 <https://reviews.llvm.org/D65386#1609956>, @labath wrote:
>
> >
>
>
> The setter function still seems more verbose than three short lines, but I don't have a strong opinion about that so setters are fine. Still, if we generate some function calling the appropriate setters, we will end up with control flow going through that function which I would like to avoid. Grepping for `SetOptionGlobal` for example would not yield any call location when people try to understand who calls these methods (which is one of the things that @jingham says he's worried about, correct me if I'm wrong).


I think we understand @jingham's concerns the same way. My answer to that would be that you'd find "SetOptionGlobal" somewhere in the tablegen file, which should be enough to signal that something tablegen-y is going on..

> 
> 
>> 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(???)) {
>>   ...
>>   }
> 
> It fails on unrecognized options (which is currently handled in every single command separately). If we can lift this up (I hope every command actually handles invalid options in the same name) then the switch-syntax should be possible.

Hmm.. how hard would it be to achieve that? If we can make that happen, then I think this might be the most reasonable compromise...

In fact, how sure are you that the unrecognised options are not already handled somewhere higher up? Doesn't all parsing already go through Options::Parse, which already checks for `'?'` (and other error-ish results from getopt). After that point, I'd hope that we can assume that the returned value does indeed correspond to some entry in our option vector...

>> 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..
> 
> I think I phrased that badly. I meant that it's a workaround until the patch is ready to land, not the usual temporary fix(TM) :)

Ah, ok, thanks for explaining that. I guess that would avoid generating code completely, but I think we should still try find a way to streamline the code that the user writes.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D65386





More information about the lldb-commits mailing list