lgtm.

lgtm

lgtm with a few minor changes.

I think this is looking good, but would like either @dexonsmith, @akyrtzi, or someone else familiar with this area to take a look. Only other comment I have is that the new functions you add should use the current LLVM naming convention.

lgtm.

I think this is fine given that we already have a copy constructor with deep copy semantics.

lgtm

lgtm

lgtm

Thanks for the cleanup. Code makes more sense now.

LGTM

Looks fine to me, but it would be good for a Swift person to look at this patch.

LGTM

Needs an example in the "Creating new Command Line Option" section, but with that it looks good.

In D95514#2528324, @jansvoboda11 wrote:

In 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?

In D94472#2519838, @jansvoboda11 wrote:

In D94472#2508018, @dexonsmith wrote:

strict mode additionally uses the GeneratedArgs1 to fill CompilerInvocation, indirectly checking both directions by requiring tests to pass both with and without this round-trip. However, during development people generally only run the tests one way and the failure mode won't be ideal.

So people build without assertions during development? In that case, I agree that erroring out on GeneratedArgs1 != GeneratedArgs2 (in all kinds of builds) would improve the experience. I don't think there's anything preventing us to incorporate this into the current patch.

LGTM.

LGTM.

LGTM.

LGTM.

LGTM with the comment.

LGTM.

LGTM.

LGTM.

lgtm

We don't really have anywhere specific to put extra matchers, and we don't want to add stuff to the actual gtest folder so it's easy to merge updates. So this is fine.

LGTM

This looks fine to be as that code is definitely dead. I would wait for @SjoerdMeijer in case there's a bug in the existing code.

lgtm

lgtm.

lgtm with nit.

lgtm with the fix above.

lgtm with the comment.

lgtm with the test fix.

lgtm.

lgtm. Thanks for the patch.

LGTM. I don't see any reason to always emit this option.

lgtm.

LGTM

In D84050#2315437, @dexonsmith wrote:

In D84050#2242755, @arsenm wrote:

In D84050#2189370, @bkramer wrote:

The parser used to rely on the \0 being present. Was that fixed?

Do you know where this assumption was? I couldn't find it, but also couldn't find any commits that seemed to "fix" it. I haven't seen a sanitizer error yet

I just did a quick scan and I can't find any such assumption either. @Bigcheese, do remember what the original intent was?

LGTM.

LGTM

LGTM

lgtm.

lgtm.

lgtm

lgtm

lgtm

lgtm