Yea, all I did was copy the code and reduce the indent level. There are other issues with the code that are non-conformant for LLVM style, but clang-format can catch all of those. It can't early indent your code for you :)
On Fri, Aug 26, 2016 at 6:17 PM Mehdi Amini <mehdi.am...@apple.com> wrote: > On Aug 26, 2016, at 6:12 PM, Zachary Turner via lldb-dev < > lldb-dev@lists.llvm.org> wrote: > > Back to the formatting issue, there's a lot of code that's going to look > bad after the reformat, because we have some DEEPLY indented code. LLVM > has adopted the early return model for this reason. A huge amount of our > deeply nested code could be solved by using early returns. For example, > here's some code in a function I was just looking at: > > // The 'A' packet is the most over designed packet ever here with > // redundant argument indexes, redundant argument lengths and needed > hex > // encoded argument string values. Really all that is needed is a comma > // separated hex encoded argument value list, but we will stay true to > the > // documented version of the 'A' packet here... > > Log *log (GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS)); > int actual_arg_index = 0; > > packet.SetFilePos(1); // Skip the 'A' > bool success = true; > while (success && packet.GetBytesLeft() > 0) > { > // Decode the decimal argument string length. This length is the > // number of hex nibbles in the argument string value. > const uint32_t arg_len = packet.GetU32(UINT32_MAX); > if (arg_len == UINT32_MAX) > success = false; > else > { > // Make sure the argument hex string length is followed by a > comma > if (packet.GetChar() != ',') > success = false; > else > { > // Decode the argument index. We ignore this really because > // who would really send down the arguments in a random > order??? > const uint32_t arg_idx = packet.GetU32(UINT32_MAX); > if (arg_idx == UINT32_MAX) > success = false; > else > { > // Make sure the argument index is followed by a comma > if (packet.GetChar() != ',') > success = false; > else > { > // Decode the argument string value from hex bytes > // back into a UTF8 string and make sure the length > // matches the one supplied in the packet > std::string arg; > if (packet.GetHexByteStringFixedLength(arg, > arg_len) != (arg_len / 2)) > success = false; > else > { > // If there are any bytes left > if (packet.GetBytesLeft()) > { > if (packet.GetChar() != ',') > success = false; > } > > if (success) > { > if (arg_idx == 0) > > m_process_launch_info.GetExecutableFile().SetFile(arg.c_str(), false); > > m_process_launch_info.GetArguments().AppendArgument(arg.c_str()); > if (log) > log->Printf ("LLGSPacketHandler::%s > added arg %d: \"%s\"", __FUNCTION__, actual_arg_index, arg.c_str ()); > ++actual_arg_index; > } > } > } > } > } > } > } > > > > Whether we like early return or not, it is the LLVM Style, and when you > have to deal with an 80 column wrapping limitation, it definitely helps. > For example, the above function becomes: > > > Since you’re going with the LLVM style, just a few notes (maybe you > quickly added the early return without clang-formatting): > > > // The 'A' packet is the most over designed packet ever here with > // redundant argument indexes, redundant argument lengths and needed > hex > // encoded argument string values. Really all that is needed is a comma > // separated hex encoded argument value list, but we will stay true to > the > // documented version of the 'A' packet here... > > Log *log (GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS)); > int actual_arg_index = 0; > > packet.SetFilePos(1); // Skip the 'A' > while (packet.GetBytesLeft() > 0) > { > > > Actually I believe the opening bracket here should be on the same line as > the while. > > // Decode the decimal argument string length. This length is the > // number of hex nibbles in the argument string value. > const uint32_t arg_len = packet.GetU32(UINT32_MAX); > if (arg_len == UINT32_MAX) > > > Any reason the if isn’t on the same indentation as the previous line? (As > for almost every other if apparently) > > return false; > // Make sure the argument hex string length is followed by a comma > if (packet.GetChar() != ',') > return false; > > // Decode the argument index. We ignore this really because > // who would really send down the arguments in a random order??? > const uint32_t arg_idx = packet.GetU32(UINT32_MAX); > if (arg_idx == UINT32_MAX) > return false; > > // Make sure the argument index is followed by a comma > if (packet.GetChar() != ',') > return false; > // Decode the argument string value from hex bytes > // back into a UTF8 string and make sure the length > // matches the one supplied in the packet > std::string arg; > if (packet.GetHexByteStringFixedLength(arg, arg_len) != (arg_len / 2)) > return false; > // If there are any bytes left > if (packet.GetBytesLeft()) > { > if (packet.GetChar() != ',') > return false; > } > > > No brackets. > > > if (arg_idx == 0) > m_process_launch_info.GetExecutableFile().SetFile(arg.c_str(), > false); > m_process_launch_info.GetArguments().AppendArgument(arg.c_str()); > if (log) > log->Printf ("LLGSPacketHandler::%s added arg %d: \"%s\"", > __FUNCTION__, actual_arg_index, arg.c_str ()); > ++actual_arg_index; > } > > This saves 24 columns!, which is 30% of the usable screen space in an > 80-column environment. > > > That’s a great motivating example to illustrate the “early return” > motivation in llvm. > > — > Mehdi > > > > On Thu, Aug 25, 2016 at 1:05 PM Zachary Turner <ztur...@google.com> wrote: > >> Definitely agree we can't map everything to that model. I can imagine a >> first step towards lit being where all our tests are literally exactly the >> same as they are today, with Makefiles and all, but where lit is used >> purely to recurse the directory tree, run things in multiple processes, and >> spawn workers. >> >> Lit should be flexible enough to support this. >> >> As a further step, imagine rewriting most tests as inline tests like >> lldbinline. Lit can support this too, the parsing and file commands are all >> up to the implementation. So the existing model of run tool and grep output >> is just one implementation of that, but you could design one that has build >> commands, c++ source, and Python all in one file, or even intermingled like >> in an lldbinline test >> >> >> On Thu, Aug 25, 2016 at 12:54 PM Todd Fiala <todd.fi...@gmail.com> wrote: >> >>> On Mon, Aug 8, 2016 at 2:57 PM, Zachary Turner via lldb-dev < >>> lldb-dev@lists.llvm.org> wrote: >>> >>>> >>>> >>>> On Mon, Aug 8, 2016 at 2:40 PM Kate Stone via lldb-dev < >>>> lldb-dev@lists.llvm.org> wrote: >>>> >>>>> LLDB has come a long way since the project was first announced. As a >>>>> robust debugger for C-family languages and Swift, LLDB is constantly in >>>>> use >>>>> by millions of developers. It has also become a foundation for bringing >>>>> up >>>>> debugger support for other languages like Go and RenderScript. In >>>>> addition >>>>> to the original macOS implementation the Linux LLDB port is in active use >>>>> and Windows support has made significant strides. IDE and editor >>>>> integration via both SB APIs and MI have made LLDB available to even more >>>>> users. It’s definitely a project every contributor can be proud of and >>>>> I’d >>>>> like to take a moment to thank everyone who has been involved in one way >>>>> or >>>>> another. >>>>> >>>>> It’s also a project that shows some signs of strain due to its rapid >>>>> growth. We’ve accumulated some technical debt that must be paid off, and >>>>> in general it seems like a good time to reflect on where we'll be headed >>>>> next. We’ve outlined a few goals for discussion below as well as one more >>>>> short-term action. Discussion is very much encouraged. >>>>> >>>>> *Forward-Looking Goals* >>>>> >>>>> 1. Testing Strategy Evaluation >>>>> >>>>> Keeping our code base healthy is next to impossible without a robust >>>>> testing strategy. Our existing suite of tests is straightforward to run >>>>> locally, and serves as a foundation for continuous integration. That >>>>> said, >>>>> it is definitely not exhaustive. Obvious priorities for improvement >>>>> include gathering coverage information, investing in more conventional >>>>> unit >>>>> tests in addition to the suite of end-to-end tests, and introducing tests >>>>> in code bases where we depend on debugger-specific behavior (e.g.: for >>>>> expression evaluation.) >>>>> >>>>> I know this is going to be controversial, but I think we should at >>>> least do a serious evaluation of whether using the lit infrastructure would >>>> work for LLDB. Conventional wisdom is that it won't work because LLDB >>>> tests are fundamentally different than LLVM tests. I actually completely >>>> agree with the latter part. They are fundamentally different. >>>> >>>> However, we've seen some effort to move towards lldb inline tests, and >>>> in a sense that's conceptually exactly what lit tests are. My concern is >>>> that nobody with experience working on LLDB has a sufficient understanding >>>> of what lit is capable of to really figure this out. >>>> >>>> I know when I mentioned this some months ago Jonathan Roelofs chimed in >>>> and said that he believes lit is extensible enough to support LLDB's use >>>> case. The argument -- if I remember it correctly -- is that the >>>> traditional view of what a lit test (i.e. a sequence of commands that >>>> checks the output of a program against expected output) is one particular >>>> implementation of a lit-style test. But that you can make your own which >>>> do whatever you want. >>>> >>>> >>> I have some interest in looking into this. Kate and I had talked about >>> it as a possible investigation back a few months ago. I'd be happy if we >>> could reduce the overall complexity of building high quality tests. I'd be >>> particularly interested in learning more about the alternative workflow >>> that isn't just "run/check/run/check", as I don't think we can map >>> everything to that model. I may take an action item on this in the near >>> future. >>> >>> >>>> This would not just be busy work either. I think everyone involved >>>> with LLDB has experienced flakiness in the test suite. Sometimes it's >>>> flakiness in LLDB itself, but sometimes it is flakiness in the test >>>> infrastructure. It would be nice to completely eliminate one source of >>>> flakiness. >>>> >>>> I think it would be worth having some LLDB experts sit down in person >>>> with some lit experts and brainstorm ways to make LLDB use lit. >>>> >>>> Certainly it's worth a serious look, even if nothing comes of it. >>>> >>>> >>>> >>>>> 4. Good Citizenship in the LLVM Community >>>>> >>>>> Last, but definitely not least, LLDB should endeavor to be a good >>>>> citizen of the LLVM community. We should encourage developers to think of >>>>> the technology stack as a coherent effort, where common code should be >>>>> introduced at an appropriate level in the stack. Opportunities to factor >>>>> reusable aspects of the LLDB code base up the stack into LLVM will be >>>>> pursued. >>>>> >>>>> One arbitrary source of inconsistency at present is LLDB’s coding >>>>> standard. That brings us to… >>>>> >>>>> *Near-Term Goal: Standardizing on LLVM-style clang-format Rules* >>>>> >>>>> We’ve heard from several in the community that would prefer to have a >>>>> single code formatting style to further unify the two code bases. Using >>>>> clang-format with the default LLVM conventions would simplify code >>>>> migration, editor configuration, and coding habits for developers who work >>>>> in multiple LLVM projects. There are non-trivial implications to >>>>> reformatting a code base with this much history. It can obfuscate history >>>>> and impact downstream projects by complicating merges. Ideally, it should >>>>> be done once with as much advance notice as is practical. Here’s the >>>>> timeline we’re proposing: >>>>> >>>>> *Today* - mechanical reformatting proposed, comment period begins >>>>> >>>>> To get a preview of what straightforward reformatting of the code >>>>> looks like, just follow these steps to get a clean copy of the repository >>>>> and reformat it: >>>>> >>>>> >>>>> 1. Check out a clean copy of the existing repository >>>>> 2. Edit .clang-format in the root of the tree, remove all but the >>>>> line “BasedOnStyle: LLVM” >>>>> 3. Change your current working directory to the root of the tree >>>>> to reformat >>>>> 4. Double-check to make sure you did step 3 ;-) >>>>> 5. Run the following shell command: "find . -name "*.[c,cpp,h] >>>>> -exec clang-format -i {} +" >>>>> >>>>> Very excited about this one, personally. While I have my share of >>>> qualms with LLVM's style, the benefit of having consistency is hard to >>>> overstate. It greatly reduces the effort to switch between codebases, a >>>> direct consequence of which is that it encourages people with LLVM >>>> expertise to jump into the LLDB codebase, which hopefully can help to tear >>>> down the invisible wall between the two. >>>> >>>> As a personal aside, this allows me to go back to my normal workflow of >>>> having 3 edit source files opened simultaneously and tiled horizontally, >>>> which is very nice. >>>> >>>> >>>> _______________________________________________ >>>> lldb-dev mailing list >>>> lldb-dev@lists.llvm.org >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev >>>> >>>> >>> >>> >>> -- >>> -Todd >>> >> _______________________________________________ > lldb-dev mailing list > lldb-dev@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev > >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev