> On Aug 26, 2016, at 6:12 PM, Zachary Turner via lldb-dev
> <[email protected]> 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 <[email protected]
> <mailto:[email protected]>> 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 <[email protected]
> <mailto:[email protected]>> wrote:
> On Mon, Aug 8, 2016 at 2:57 PM, Zachary Turner via lldb-dev
> <[email protected] <mailto:[email protected]>> wrote:
>
>
> On Mon, Aug 8, 2016 at 2:40 PM Kate Stone via lldb-dev
> <[email protected] <mailto:[email protected]>> 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:
> Check out a clean copy of the existing repository
> Edit .clang-format in the root of the tree, remove all but the line
> “BasedOnStyle: LLVM”
> Change your current working directory to the root of the tree to reformat
> Double-check to make sure you did step 3 ;-)
> 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
> [email protected] <mailto:[email protected]>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> <http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev>
>
>
>
>
> --
> -Todd
> _______________________________________________
> lldb-dev mailing list
> [email protected]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
_______________________________________________
lldb-dev mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
