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

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added inline comments.


================
Comment at: lldb/source/Interpreter/OptionGroupFormat.cpp:66
+           0,
+           eArgTypeCount,
+           "The number of total items to display."},
----------------
DavidSpickett wrote:
> 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.
Ah, I could re-use the option table. Just an oversite from my end. Thank you for spotting that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114448