[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Thanks for splitting the patches.



================
Comment at: lldb/source/Interpreter/OptionGroupFormat.cpp:30
+  m_option_definition[2] = default_opt_defs[2];
+  m_option_definition[3] = default_opt_defs[3];
 
----------------
You could use a std::copy for this and not need to spell out each index.


================
Comment at: lldb/source/Interpreter/OptionGroupFormat.cpp:66
+           0,
+           eArgTypeCount,
+           "The number of total items to display."},
----------------
RamNalamothu wrote:
> DavidSpickett wrote:
> > Not sure if clang-format has done this, or just your preferred style. Nothing against either but it makes it difficult to tell if anything has changed in this particular part of the diff.
> > 
> > In general we want clang-formatted code but if it makes the diff tricky to read it's best done in a follow up change that just does formatting.
> The clang-format did it. Will mark this with `clang-format off/on`.
My bad, what I meant was that it's fine to format it in general but that in this case it should be avoided to make the diff more readable.

But now I see that this is essentially a new function so formatting it all is fine. I now see the change you're making, the body of the array/function isn't actually changing. You can remove the `/* clang-format...*/` and just let it do its thing.

That said, what is the reason that you couldn't re-use the option table?

I have two thoughts about the function:
1. Do we need to remake the array each time, vs the constexpr array from before.
2. We're returning an ArrayRef to a return value, which probably works in some situations (if you immediately do a copy using the ref, then discard it) but I think it's a lifetime issue overall.

I'm assuming that ArrayRef is basically std::array& in a general sense, and if you returned a std::array& to something on the stack, that's a lifetime issue for sure. I looked at some other option groups and they all follow the pattern of returning an array ref to a const array defined outside the function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114448