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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
