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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
