[Lldb-commits] [PATCH] D145574: [lldb] Read register fields from target XML

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4053
+      // appears once, so we don't have to handle that here.
+      if (attr_name == "name") {
+        LLDB_LOGF(log,
----------------
DavidSpickett wrote:
> JDevlieghere wrote:
> > Do you think that using a `StringSwitch` could improve readability? 
> Unless I have the wrong end of the stick, StringSwitch is usually about returning a single value. Here we'd return a lambda for processing the attribute value.
> 
> That would look like:
> ```
>       std::function<void(llvm::StringRef)> processor =
>           llvm::StringSwitch<std::function<void(llvm::StringRef)>>(attr_name)
>               .Case("name",
>                     [&name, log](llvm::StringRef value) {
>                       LLDB_LOGF(log,
>                                 "ProcessGDBRemote::ParseFlags Found field node "
>                                 "name \"%s\"",
>                                 value.data());
>                       name = value;
>                     })
>               .Case("start",
>                     [&start, log, max_start_bit](llvm::StringRef value) {
>                       unsigned parsed_start = 0;
>                       if (llvm::to_integer(value, parsed_start)) {
>                         if (parsed_start > max_start_bit) {
>                           LLDB_LOGF(log,
>                                     "ProcessGDBRemote::ParseFlags Invalid "
>                                     "start %u in field node, "
>                                     "cannot be > %u",
>                                     parsed_start, max_start_bit);
>                         } else
>                           start = parsed_start;
>                       } else {
>                         LLDB_LOGF(log,
>                                   "ProcessGDBRemote::ParseFlags Invalid start "
>                                   "\"%s\" in field node",
>                                   value.data());
>                       }
>                     });
> 
>        processor(value);
> ```
> 
> Which seems about the same to me assuming one is comfortable reading lambdas, but tell me what you think. The extra indents seem to help more than the StringSwitch itself.
Fair enough, and it's probably not worth creating an enum for this (which is what I originally had in mind). I think the code is fine as it is. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145574