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