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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
teemperor added a reviewer: JDevlieghere.
Herald added subscribers: lldb-commits, abidh.
Herald added a project: LLDB.
teemperor added a comment.

For the sake of completeness, that's how Clang would warn about unimplemented options:

  llvm-project/lldb/source/Commands/CommandObjectSettings.cpp:102:15: warning: enumeration value 'Global' not handled in switch [-Wswitch]
        switch (*opt) {
                ^
  llvm-project/lldb/source/Commands/CommandObjectSettings.cpp:102:15: note: add missing switch cases
        switch (*opt) {
                ^


Currently in LLDB we handle options like this:

  switch(short_option) {
  case 'g': m_force = true; break;
  case 'f': m_global = true; supportGlobal(); break;
  default:
    error.SetErrorStringWithFormat("unrecognized options '%c'",
                                         short_option);
    break;

This format has a two problems:

1. If we don't handle an option in this switch statement (because of a typo, a changed short-option

name, or because someone just forgot to implement it), we only find out when we have a test or user
that notices we get an error when using this option. There is no compile-time verification of this.

2. It's not very easy to read unless you know all the short options for all commands.

This patch makes our tablegen emit an enum that represents the options, which changes the code above to this (using SettingsSet as an example):

  auto opt = toSettingsSetEnum(short_option, error);
  if (!opt)
    return error;
  
  switch (opt) {
  case SettingsSet::Global: m_force = true; break;
  case SettingsSet::Force: m_global = true; supportGlobal(); break;
  // No default with error handling, already handled in toSettingsSetEnum.
  // If you don't implement an option, this will trigger a compiler warning for unhandled enum value.
  }



NOTE: This is just a dummy patch to get some feedback if people are for some reason opposed to this change
(which would save me from converting all our switch-cases to the new format). I only changed the code for
`settings set` to demonstrate the change.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D65386

Files:
  lldb/source/Commands/CommandObjectSettings.cpp
  lldb/utils/TableGen/LLDBOptionDefEmitter.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D65386.212134.patch
Type: text/x-patch
Size: 3708 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20190729/828d8195/attachment.bin>