[PATCH] D79262: [COFF] Add and use a zero-copy tokenizer for .drectve

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked an inline comment as done.
rnk added a comment.

In D79262#2015645 <https://reviews.llvm.org/D79262#2015645>, @thakis wrote:

> How involved would it be to move all callers to this new style api (posix too, I suppose)?


Very involved. I tried it. :)

The tokenizing API is mostly used for response file expansion, which is done before flag parsing. Both LLVM command line parsing libraries expect to see C strings, because they are set up to work on a raw argv list. I started StringRef-izing them, but it never ends.

The other thing is that, if we did zero-copy response file expansion, we would need to extend the lifetime of the response file MemoryBuffers. This is kind of cool in that we save one copy, but normally we read the response file and then throw away the memory, so there is no total reduction memory usage from updating the API.

Lastly, there is some inherent risk to passing around non-null-terminated StringRefs. Someone might call `.data()` on it to get a C string, but if it comes from a response file, it won't be one.



================
Comment at: llvm/lib/Support/CommandLine.cpp:973
+      if (isWhitespaceOrNull(Src[I])) {
+        AddToken(Saver.save(Token.str()));
         Token.clear();
----------------
thakis wrote:
> nit: Maybe add a comment along the lines of "// The token required unquoting, so it must always be save, regardless of AlwaysCopy".
Will do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79262/new/

https://reviews.llvm.org/D79262