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

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

In D95514#2528324 <https://reviews.llvm.org/D95514#2528324>, @jansvoboda11 wrote:

> In D95514#2526064 <https://reviews.llvm.org/D95514#2526064>, @dexonsmith wrote:
>
>> Well, the compiler developers are the users, since `-cc1` isn't for end-users. I acknowledge it's a tradeoff. IMO, without actually seeing it in practice, it seems a bit nicer to design this away entirely. But maybe having a `=` is more valuable than I think.
>
>
>
>> For string options:
>>
>> - If `-cc1` accepts only `-opt string`, no need for allocation.
>> - If `-cc1` accepts only `-opt=string`, the fully-spelled option is on the original command-line. `ExistingStrings.find()` should turn it up when calling `Strings.save()`.
>> - If `-cc1` accepts both `-opt=string` and `-opt string` canonicalize to `-opt string` when generating. No need for allocation.
>>
>> For numeric / enum options we'll need to allocate, but the allocation doesn't have to live past running the parser.
>>
>> The only problem would be if `string` gets canonicalized / rewritten somehow during parse + generate, or if an enum value is additionally saved as a string value.
>
> We can definitely ask around how would people feel about that.
>
>>>> 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.
>>
>> Maybe it wouldn't be terrible to have a `RoundTrip` arg on each options class. When you assign to the CompilerInvocation, it copies the shared ptr into each of the options classes, so that the pool stays alive as long as one of them has it.
>
> I think this is the most robust solution.
>
> Moving `BumpPtrAllocator`/`StringSaver` into `CompilerInvocation` will become problematic once we start round-tripping more option classes.
>
> I've looked through all option classes in `CompilerInvocation`, and `AnalyzerOptions::Config` seems to be the only member that stores non-owning references to command line arguments.
> I think the best path forward is to change `AnalyzerOptions` to take ownership and avoid dealing with complicated lifetimes. We can look into removing allocations later, as an optional optimization.
> What do you think?

I think that makes sense given the rest of `CompilerInvocation` is owning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95514