In D127182#3582540, @jansvoboda11 wrote:

Should we add a FIXME saying we need to handle other kinds of attributes?

lgtm.

This does require build systems to be able to totally replace a command line rather than just add to it, but it seems like that's a requirement unless we add a way to modify the command line to Clang. lgtm.

lgtm. I agree that testing this isn't really necessary given the difficulty of even constructing one.

lgtm

• Fixed documentation typo.
• Made missing '{' diagnostic more clear.
• Use ModuleMapParser::skipUntil.

Drive-by comment on the docs; otherwise this sounds awesome; as long as else is easy to add later this SGTM (I'll let others do the code review).
(Although, if else {} and else requires {} would be easy to add now/soon, I feel like it'd be worth it. Modelling an else-requires chain by hand would be quite error-prone, and it might be annoying to stage the adoption separately...)

Add testing of empty blocks and nested blocks and make the missing { error clearer.

Is it possible to reference external module map from requires block? I mean that in one context the module has some extra requirements but in a different context doesn't have them.

lgtm

In D114342#3267957, @tschuett wrote:

I though modern LLVM is to return llvm::Expected<std::string>?!?

Once these issues are fixed this should be fine.

In D114206#3221893, @lenary wrote:

Update: I'm going to revert this on main. This patch was accepted conditionally on you fixing the windows crash, which you have not done, so this was landed without being accepted.

Revert is here: https://reviews.llvm.org/rGea835171389a

When you re-land this, don't forget to include the changes from D116611, please.

Can we not consider a modulemap used when we load an AST that depends on that modulemap? It's possible this breaks if the module only includes textual headers though. It really feels like we should have a single place where we actually know if a module is used or not. Long term I would really like to separate modulemap parsing from Module creation, which would be a great place to actually do this.

There were two issues for Windows.

I think this makes sense to do given that you need to explicitly opt into inferred modules anyway. This isn't implicitly finding module maps, as we already have a module map with framework module *.

lgtm

lgtm

lgtm

Looks good. We shouldn't be checking _WIN32 just for path style questions.

Looks reasonable. Edges with weight 0 shouldn't matter anyway.

Here's a version that actually works (python 3, not sure if it's valid in 2), although I would much prefer we not write to the source directory during a build.

Also, this python script just doesn't work. It's missing a sys import, a "w" flag, and a new line after each write. It also dumps the output into the source directory. Was this actually tested?

For future reference this was very difficult to merge into external changes. It looks like you resorted this at the same time as renaming it, and that messes up git's rename logic.

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