<div dir="ltr">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 :)</div><br><div class="gmail_quote"><div dir="ltr">On Fri, Aug 26, 2016 at 6:17 PM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><blockquote type="cite"><div>On Aug 26, 2016, at 6:12 PM, Zachary Turner via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>> wrote:</div><br class="m_-8898744346157660478Apple-interchange-newline"><div><div dir="ltr">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:<div><br></div><div><div> // The 'A' packet is the most over designed packet ever here with</div><div> // redundant argument indexes, redundant argument lengths and needed hex</div><div> // encoded argument string values. Really all that is needed is a comma</div><div> // separated hex encoded argument value list, but we will stay true to the</div><div> // documented version of the 'A' packet here...</div><div><br></div><div> Log *log (GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));</div><div> int actual_arg_index = 0;</div><div><br></div><div> packet.SetFilePos(1); // Skip the 'A'</div><div> bool success = true;</div><div> while (success && packet.GetBytesLeft() > 0)</div><div> {</div><div> // Decode the decimal argument string length. This length is the</div><div> // number of hex nibbles in the argument string value.</div><div> const uint32_t arg_len = packet.GetU32(UINT32_MAX);</div><div> if (arg_len == UINT32_MAX)</div><div> success = false;</div><div> else</div><div> {</div><div> // Make sure the argument hex string length is followed by a comma</div><div> if (packet.GetChar() != ',')</div><div> success = false;</div><div> else</div><div> {</div><div> // Decode the argument index. We ignore this really because</div><div> // who would really send down the arguments in a random order???</div><div> const uint32_t arg_idx = packet.GetU32(UINT32_MAX);</div><div> if (arg_idx == UINT32_MAX)</div><div> success = false;</div><div> else</div><div> {</div><div> // Make sure the argument index is followed by a comma</div><div> if (packet.GetChar() != ',')</div><div> success = false;</div><div> else</div><div> {</div><div> // Decode the argument string value from hex bytes</div><div> // back into a UTF8 string and make sure the length</div><div> // matches the one supplied in the packet</div><div> std::string arg;</div><div> if (packet.GetHexByteStringFixedLength(arg, arg_len) != (arg_len / 2))</div><div> success = false;</div><div> else</div><div> {</div><div> // If there are any bytes left</div><div> if (packet.GetBytesLeft())</div><div> {</div><div> if (packet.GetChar() != ',')</div><div> success = false;</div><div> }</div><div><br></div><div> if (success)</div><div> {</div><div> if (arg_idx == 0)</div><div> m_process_launch_info.GetExecutableFile().SetFile(arg.c_str(), false);</div><div> m_process_launch_info.GetArguments().AppendArgument(arg.c_str());</div><div> if (log)</div><div> log->Printf ("LLGSPacketHandler::%s added arg %d: \"%s\"", __FUNCTION__, actual_arg_index, arg.c_str ());</div><div> ++actual_arg_index;</div><div> }</div><div> }</div><div> }</div><div> }</div><div> }</div><div> }</div><div> }</div></div><div><br></div><div><br></div><div><br></div><div>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:</div></div></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word"><div><div>Since you’re going with the LLVM style, just a few notes (maybe you quickly added the early return without clang-formatting): </div></div></div><div style="word-wrap:break-word"><div><div><br></div><blockquote type="cite"><div><div dir="ltr"><div><br></div><div><div> // The 'A' packet is the most over designed packet ever here with</div><div> // redundant argument indexes, redundant argument lengths and needed hex</div><div> // encoded argument string values. Really all that is needed is a comma</div><div> // separated hex encoded argument value list, but we will stay true to the</div><div> // documented version of the 'A' packet here...</div><div><br></div><div> Log *log (GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));</div><div> int actual_arg_index = 0;</div><div><br></div><div> packet.SetFilePos(1); // Skip the 'A'</div><div> while (packet.GetBytesLeft() > 0)</div><div> {</div></div></div></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word"><div><div>Actually I believe the opening bracket here should be on the same line as the while.</div></div></div><div style="word-wrap:break-word"><div><br><blockquote type="cite"><div><div dir="ltr"><div><div> // Decode the decimal argument string length. This length is the</div><div> // number of hex nibbles in the argument string value.</div><div> const uint32_t arg_len = packet.GetU32(UINT32_MAX);</div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span>if (arg_len == UINT32_MAX)</div></div></div></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word"><div><div>Any reason the if isn’t on the same indentation as the previous line? (As for almost every other if apparently)</div></div></div><div style="word-wrap:break-word"><div><br><blockquote type="cite"><div><div dir="ltr"><div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span>return false;</div><div> // Make sure the argument hex string length is followed by a comma</div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span>if (packet.GetChar() != ',')</div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span>return false;</div><div><br></div><div> // Decode the argument index. We ignore this really because</div><div> // who would really send down the arguments in a random order???</div><div> const uint32_t arg_idx = packet.GetU32(UINT32_MAX);</div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span>if (arg_idx == UINT32_MAX)</div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span>return false;</div><div><br></div><div> // Make sure the argument index is followed by a comma</div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span>if (packet.GetChar() != ',')</div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span>return false;</div><div> // Decode the argument string value from hex bytes</div><div> // back into a UTF8 string and make sure the length</div><div> // matches the one supplied in the packet</div><div> std::string arg;</div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span>if (packet.GetHexByteStringFixedLength(arg, arg_len) != (arg_len / 2))</div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span>return false;</div><div> // If there are any bytes left</div><div> if (packet.GetBytesLeft())</div><div> {</div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span> if (packet.GetChar() != ',')</div><div><span class="m_-8898744346157660478Apple-tab-span" style="white-space:pre-wrap"> </span> return false;</div><div> }</div></div></div></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word"><div><div>No brackets.</div></div></div><div style="word-wrap:break-word"><div><br><blockquote type="cite"><div><div dir="ltr"><div><div><br></div><div> if (arg_idx == 0)</div><div> m_process_launch_info.GetExecutableFile().SetFile(arg.c_str(), false);</div><div> m_process_launch_info.GetArguments().AppendArgument(arg.c_str());</div><div> if (log)</div><div> log->Printf ("LLGSPacketHandler::%s added arg %d: \"%s\"", __FUNCTION__, actual_arg_index, arg.c_str ());</div><div> ++actual_arg_index;</div><div> }</div></div><div><br></div><div>This saves 24 columns!, which is 30% of the usable screen space in an 80-column environment.</div></div></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word"><div><div>That’s a great motivating example to illustrate the “early return” motivation in llvm.</div><div><br></div><div>— </div></div></div><div style="word-wrap:break-word"><div><div>Mehdi</div></div></div><div style="word-wrap:break-word"><div><div><br></div><br><blockquote type="cite"><div><br><div class="gmail_quote"><div dir="ltr">On Thu, Aug 25, 2016 at 1:05 PM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br><br>Lit should be flexible enough to support this.<br><br>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<br><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Aug 25, 2016 at 12:54 PM Todd Fiala <<a href="mailto:todd.fiala@gmail.com" target="_blank">todd.fiala@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Aug 8, 2016 at 2:57 PM, Zachary Turner via lldb-dev <span dir="ltr"><<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span><div dir="ltr">On Mon, Aug 8, 2016 at 2:40 PM Kate Stone via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div style="font-family:HelveticaNeue">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.</div><div style="font-family:HelveticaNeue"><br></div><div style="font-family:HelveticaNeue">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.</div><div style="font-family:HelveticaNeue"><br></div><div style="font-family:HelveticaNeue"><b>Forward-Looking Goals</b></div><div style="font-family:HelveticaNeue"><br></div><div style="font-family:HelveticaNeue"> 1.<span style="white-space:pre-wrap"> </span>Testing Strategy Evaluation</div><div style="font-family:HelveticaNeue"><br></div></div><blockquote style="font-family:HelveticaNeue;margin:0px 0px 0px 40px;border:none;padding:0px">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.)</blockquote></div></blockquote></span><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div></div><div>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.</div><div><br></div><div>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. </div><div><br></div><div>Certainly it's worth a serious look, even if nothing comes of it.</div><span><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div style="font-family:HelveticaNeue"><div><div><div> 4.<span style="white-space:pre-wrap"> </span>Good Citizenship in the LLVM Community</div><div><br></div></div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div>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.</div><div><br></div><div>One arbitrary source of inconsistency at present is LLDB’s coding standard. That brings us to…</div><div><br></div></blockquote><div></div></div></div><div style="font-family:HelveticaNeue"><div><b>Near-Term Goal: Standardizing on LLVM-style clang-format Rules</b></div><div><br></div><div>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:</div><div><br></div><div><b>Today</b> - mechanical reformatting proposed, comment period begins</div><div><br></div></div><blockquote style="font-family:HelveticaNeue;margin:0px 0px 0px 40px;border:none;padding:0px">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:</blockquote><blockquote style="font-family:HelveticaNeue;margin:0px 0px 0px 40px;border:none;padding:0px"><ol><li>Check out a clean copy of the existing repository</li><li>Edit .clang-format in the root of the tree, remove all but the line “BasedOnStyle: LLVM”</li><li>Change your current working directory to the root of the tree to reformat</li><li>Double-check to make sure you did step 3 ;-)</li><li>Run the following shell command: "find . -name "*.[c,cpp,h] -exec clang-format -i {} +"</li></ol></blockquote></div></blockquote></span><div>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.</div><div><br></div><div>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.<span style="line-height:1.5"> </span></div><div><br></div></div></div>
<br></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">_______________________________________________<br>
lldb-dev mailing list<br>
<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a><br>
<br></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><br><br clear="all"><div><br></div>-- <br><div class="m_-8898744346157660478m_-4612345979865539240m_8389219872109007441gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">-Todd</div></div>
</div></div></blockquote></div></blockquote></div>
_______________________________________________<br>lldb-dev mailing list<br><a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a><br></div></blockquote></div></div></blockquote></div>