jansvoboda11 added a comment. 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? ================ Comment at: clang/include/clang/Frontend/CompilerInvocation.h:144 + /// Container holding strings allocated during round-trip. + mutable std::forward_list<std::string> AllocatedStrings; + ---------------- dexonsmith wrote: > jansvoboda11 wrote: > > dexonsmith wrote: > > > I suggest using llvm::StringPool instead, which also uniques the strings > > > and is used elsewhere for this kind of thing. Probably in an Optional to > > > make it more clear it’s not always used. > > > > > > Maybe the name RoundTrippedStrings? > > From the documentation of `StringPool`, it seems like we would need to > > store `PooledStringPtr`s in order to keep them alive. How would that work > > with the `const char *` API we have? > Oops; I meant `StringSet<>` / `StringMap<NoneType>` / etc., which is what > I've always used in the past. This is essentially a `StringMap<void>`; I just > always call the variable `StringPool`, hence my error. It has the properties > you need (stable, kept alive indefinitely, null-terminated). > > Looking around, there are a couple of options designed for this: > - `StringSaver`, which is an optimized version of your list (uses a > bumpptrallocator, no uniquing). > - `UniqueStringSaver`, which is essentially equivalent to `StringSet<>`; > might be slightly more optimized, and if it's not, we should fix it. > > I suggest using one of those since they exactly match what you need. Thanks for mentioning `StringSaver`s, they fit our needs perfectly (now used in D94472). 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
