[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Zachary Turner via lldb-commits]

zturner created this revision.
zturner added a reviewer: clayborg.
zturner added subscribers: lldb-commits, LLDB.

In https://reviews.llvm.org/D24952 I tried to refactor the `Args` class to get rid of `m_argv`.  The original motivation for this is that I wanted a way to implement `GetArgumentAtIndex()` so as to return a `StringRef`.  I could have just returned a `StringRef` from the `m_argv` array, but it seems wasteful to calculate the length every time when we were already storing a `std::string`.   Unfortunately the `std::string` was in a `std::list` (required so that it would not be invalidated when inserting items into the arg list) so there was no random access.  So I wanted to see if I could do better, and that was the motivation for the original patch.

However, after fixing up all the places in the code to use the "new" `Args`, I wasn't completely satisfied.  I like that the old code could just give you an `argv` pointer that you can pass to a system API.  So I started over and ended up with this.  Quick overview of changes:

1. Instead of using a `std::list<std::string>` to own the memory, we use a `std::vector<std::unique_ptr<char[]>>`.  This provides random access, which is something `list` couldn't provide, and it also provides non-const access to the underlying buffer, which is something that `std::string` couldn't provide, leading to a lot of `const_cast` which are no longer necessary.
2. Each entry's quote char, backing memory (i.e. `std::unique_ptr<char[]>`), and `StringRef` with precomputed length are stored together in a single entry.  This guarantees by design that the various arrays' lengths do not need to stay in sync with each other, because there is only one array.
3. Some longstanding undefined behavior is documented and left for someone else to fix.
4. Asserts are added to the code to document the pre-conditions.  I know we are allergic to asserts, but I want to emphasize that none of these asserts have anything to do with the input parameters.  The asserts are ALWAYS on internal class state.

With this change it should finally be easy to change `GetArgumentAtIndex()` to return a `StringRef`, because it can just return `m_entries[idx].ref`.


https://reviews.llvm.org/D25099

Files:
  include/lldb/Interpreter/Args.h
  source/Core/StringList.cpp
  source/Interpreter/Args.cpp
  unittests/Interpreter/TestArgs.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D25099.73064.patch
Type: text/x-patch
Size: 31632 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160930/d6f9048a/attachment-0001.bin>