LGTM for llvm/{lib,include/llvm}/Object/* and llvm/tools/llvm-readobj/*.

committed as a20862307fff9385bbf6562de3154d05138f8bec.

Sorry for the delay. LGTM. I'll commit with you as the author.

In D69575#1906078, @Ladd wrote:

@Bigcheese -- any more thoughts on this one? It would be helpful to have some version of this fix in the release compiler.

Cleaned up the test to not reference unused paths.

In D75395#1901778, @dexonsmith wrote:

This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

I had another thought. What if for the explicitly built "system" modules:

• If -Wsystem-headers is on, leave them as user modules in the explicit build.
• If -Wsystem-headers is off, turn off all diagnostics in the explicit build.
  
  Does that give the right semantics, or is there something subtly different?

I pushed a commit fixing this in rG0b6abe428164 by just excluding the header.

I'm pretty sure this commit broke the modules build on non-Windows. WindowsSupport.h is now part of the LLVM_Util module which gets built on all platforms. You need to update the modulemap to put it in its own module, or wrap it in #ifdef.

lgtm

The aforementioned review is at https://reviews.llvm.org/D74892. I'm currently verifying it fixes everything for us. I'll update you when you're good to recommit this, which should happen this week.

Confirmed that CMake has a bug here: https://github.com/Kitware/CMake/blob/master/Source/cmLocalGenerator.cxx#L1780

This change also breaks MacOS, as -arch doesn't get passed to these invocations. Possibly CMAKE_OSX_ARCHITECTURES not being handled correctly within CMake.

I would love to see one of these land, but I'll need to find some time to do a deeper look at the two.

I was finally able to land the patch this depended on. Still waiting on review for this. In the meantime I've tested this ABI out by building a simple program that outputs a ninja file given a command line. It depends on a bunch of changes to the Clang side of things, but no changes to this interface.

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.