[PATCH] D95514: [clang][cli] Teach CompilerInvocation to allocate strings on its own

Wed Jan 27 08:32:54 PST 2021

jansvoboda11 added a comment.

In D95514#2525255 <https://reviews.llvm.org/D95514#2525255>, @dexonsmith wrote:

> Can you include the use in the patch?
> I also wonder what command-line option is forcing this.

The use-case that requires it is here: https://github.com/llvm/llvm-project/blob/f3449ed6073cac58efd9b62d0eb285affa650238/clang/lib/Frontend/CompilerInvocation.cpp#L734.
Clang accepts analyzer arguments in form of `-analyzer-config key=value`. Reference to the `key` sub-string is stored as an index in a `StringMap` (`AnalyzerOptions::Config`) and values are held by `std::string`. More entries get stored into the map in `parseAnalyzerConfigs`.

> String arguments could reuse the original char*.

Interesting idea. We could avoid allocation (and the need for owning strings) by grabbing the map key substring, scanning past its end until we hit `\0` and putting the whole thing into generated arguments. Keys in the `StringMap` that are not followed by `=value` most likely only come from `parseAnalyzerConfigs`, but we don't need to generate those.
This could work, but I'm worried it's somewhat brittle.

> It seems better to drop frivolous `=` than carrying this around.

Cool, this could save us allocations in some cases. Not sure dropping `=` between key-value pairs would result in good user experience though.

> I’m not sure having the AllocateString API totally makes sense, vs moving in a string pool that was independently used during generation, but maybe in context?

What exactly do you mean here?

> Consider that some callers may parse the full command-line and then drop everything but one options class. That pattern wouldn’t work anymore since the StringRefs won’t be valid.

Ah, that's right. So if we don't go with the allocation-less approach, we'd need to take `StringAllocator` from the client that also ensures proper lifetimes.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95514