Looks like the test for -full-command-line got dropped. I'll add that when I commit.

Refactored FullDependencies::getAdditionalCommandLine and ModuleDeps::getFullCommandLine to share code.

• Remove duplicate decl
• Add test

• Removed OutputPaths.
• Add documentation for AlreadySeen.

It was already there from a previous commit. I see it in tree currently.

lgtm. Nice cleanup.

lgtm

lgtm

Thanks, you're good to commit.

In D70351#1754276, @arphaman wrote:

Adding experimental APIs is something that we haven't done before, but it be useful for this case.

lgtm as long as other compilers don't warn on unknown pragmas by default.

A few minor points, but with those fixed this looks good to me. Thanks for the patch!

lgtm with the changes to FindResourceDir.

lgtm. Looking at the blame it would be best if @JDevlieghere @vsapsai or @benlangmuir could take a look also.

lgtm, sorry for the delay, I didn't see the updates for some reason.

So I agree solving this problem makes sense, but I have some issues with the current patch.

I forgot to clang-format.

This fixes some issues I received externally.

Address review comments.

I've added Manuel as a reviewer as this patch is also changing the tooling APIs.

This was originally not added to the legacy pass manager because it leaks memory under it due to fundamental issues with the old pass manager. See https://reviews.llvm.org/D48105#1140299 .

LGTM with style nit. I like that this decouples DependencyScanningTool from printing the results.

The current assumption is that the clang-scan-deps binary is the one that comes next to the clang binary you are using. There are lots of other differences between clang versions than just the resource-dir.

While adding the documentation I realized that a better name for this option would be -fmodules-strict-context-hash to make it clear which hash it's referring to.

Fixed spelling and updated comment.

Added .i, .ii, .mi, and .mmi as files to minimize.

lgtm. Jan may want to take a look as I believe he was looking at a related issue.

Addressed review comments.

lgtm, but wait for Alex or Duncan to also take a look.

In D68193#1701426, @kousikk wrote:

@arphaman I don't mind changing this if there are race conditions as you say, but isn't the assumption of the tool that the filesystem remains unchanged for a single run of the tool? If so, should we actually throw error conditions instead of crashing in those cases?

Add diagnostics.

Do we really want to output \r\n on Windows? There is all of one program (notepad.exe) that doesn't support \n on Windows.

lgtm

Also, please use -U999999 for patches so there's full context.

Looking more into this I'm not sure you need the PPCallbacks. The Preprocessor should just own the PreprocessorSkippedMappings data structure and the dependency scanner can update it live.

lgtm