LGTM too.

In D61547#1497555, @MaskRay wrote:

In D61547#1497030, @dexonsmith wrote:

In D61547#1495855, @MaskRay wrote:

In D61547#1495339, @rnk wrote:

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

I used llvm-as without this patch to generate it. I added anither test to check the upgrader correctly adds i8* null

What's the problem with using the pre-existing file?

test/Bitcode/upgrade-global-ctors.ll.bc uses zeroinitializer, it doesn't check if AuoUpgrader adds i8* null as the third field. So I improved the test a bit and made it clear both globals_ctors and global_dtors are upgraded.

In D61547#1495855, @MaskRay wrote:

In D61547#1495339, @rnk wrote:

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

I used llvm-as without this patch to generate it. I added anither test to check the upgrader correctly adds i8* null

In D61547#1495339, @rnk wrote:

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

+bruno

Hmm; it doesn't seem safe to obsolete the 2-field form if we don't have a working bitcode upgrade from 2-field to 3-field.

In D61165#1492724, @rjmccall wrote:

In D61165#1492582, @erik.pilkington wrote:

Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which appears to claim that initialization ends with the execution of the constructor, excluding temporary destructors.

(13) In a non-delegating constructor, initialization proceeds in the following order:
/* list of steps... */
(13.4) Finally, the compound-statement of the constructor body is executed.

John: do you agree with this analysis?

That's talking about constructor bodies.

The 3-field form was introduced by D3499 in 2014 and the legacy 2-field
form was planned to be removed in LLVM 4.0

+rnk

Shouldn't this just send DYLD_LIBRARY_PATH through to the second stage, so that ar and ranlib use the just-built libLTO.dylib?

LGTM.

In D60924#1473176, @steven_wu wrote:

In D60924#1473166, @dexonsmith wrote:

In D60924#1473161, @steven_wu wrote:

In D60924#1473135, @dexonsmith wrote:

Is there a good way to test this?

Not currently in LLVM. It might be possible to add a test in clang repo which requires DARWIN and ld64.

Maybe we can split out the function to generate the filename, and add a unit test for that?

That almost looks like a test for path::append. Not sure if there is much value to add that.

In D60924#1473161, @steven_wu wrote:

In D60924#1473135, @dexonsmith wrote:

Is there a good way to test this?

Not currently in LLVM. It might be possible to add a test in clang repo which requires DARWIN and ld64.

Is there a good way to test this?

In D60748#1472829, @rjmccall wrote:

I suspect Darwin also doesn't want to take this. We care very little about 32-bit Intel, and part of caring very little is not wanting to spend any effort dealing with the ramifications of ABI breaks. That would apply both to the rule in general and to the vector rule specifically. @dexonsmith, agreed?

I wonder if we could add a module flag for the TLI, and then store that in the summary. Would it work for both implementations in that case?

+aprantl

In D60233#1467092, @trixirt wrote:

A comment by whisperity in the origin WIP did not seem to be addressed.
Have you looked at IWYU https://github.com/include-what-you-use/include-what-you-use ?
The end goals of clang-scan-deps and iwyu seem similar so their implementation problems would also be similar.
The iwyu problems doc is https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYUIsDifficult.md

In D55463#1451166, @tschuett wrote:

Are you planning to do this recursively?
The minimizer does not help much for the following example, while Sema.h contains 10,000 lines of useless code.

#include "clang/Sema/Sema.h"

int foo() {}

I'm in favour of this.

Committed r357038.

Committed r357037.

+Erik and Alex, can you take a look at this?

LGTM.

LGTM!

Rebase.

In D59377#1431203, @jkorous wrote:

In D59377#1430002, @dexonsmith wrote:

... since I noticed that FileManager ...

This kind of implies that we should move the comment from FileManager constructor implementation to the header thus making it an explicit part of the interface contract.

// If the caller doesn't provide a virtual file system, just grab the real
// file system.

Maybe https://reviews.llvm.org/D59388 would be the right place to do it.

Document the default VFS used by the FileManager.

In D59388#1431314, @jkorous wrote:

Hi Duncan, thanks for working on better interfaces in clang!

I am just wondering - is it safe to have the lifetime of a single object on heap managed by two different IntrusiveRefCntPtr instances?

I slightly simplified the code for CompilerInstance::createFileManager (replaced with an assert), since I noticed that FileManager always constructs its own VFS if it isn't passed one.

In D58514#1428567, @ahatanak wrote:

Sorry, I misread the code. If you change the parameter type of bar to BlockTy, the code crashes. If the type is id, it doesn't crash because IRGen copies the block to the heap in foo before passing it to bar.

In D58514#1428520, @ahatanak wrote:

That code doesn't crash because the block is retained at the entry of bar and ARC optimizer doesn't remove the retain/release pairs in bar.

In D58514#1428434, @ahatanak wrote:

Seems like the chromium code is valid and shouldn't crash. John/Erik what do you think? The following code also crashes with this patch applied.

typedef void (^BlockTy)();

BlockTy sb;
__weak BlockTy wb;

void foo(id a) {
  auto b = ^{ NSLog(@"foo %@", a); };
  wb = b; // block isn't copied to the heap.
  sb = b; // block is copied to the heap.
}

int main() {
  auto x = [NSObject new];
  foo(x);
  sb();
  wb();
  return 0;
}

Committed in r355950. Thanks for the review.

Updated the constructor call to PCHGenerator in GenerateModuleAction::CreateASTConsumer to use BuildingImplicitModule on its own. Checking where it's set (only in compileModuleImpl), it's exactly the condition we want here.

In D59176#1424864, @jordan_rose wrote:

This commit by itself doesn't change any behavior, right?

Committed in r355778.

Committed in r355777.

LGTM too with some nitpicks (forgot to hit "submit" earlier this afternoon).

+ab, do you still know this code?

LGTM. Thanks!

Committed in r355477.

In D58890#1417465, @bruno wrote:

InMemoryModuleCache seems like a way more appropriate name here. Also thanks for improving some of the comments.

Because of the move to Serialization we can no longer abuse the Preprocessor to forward it to the ASTReader. Besides the rename and file move, that means Preprocessor::Preprocessor has one fewer parameter and ASTReader::ASTReader has one more.

Are there any C api bits that expose preprocessor stuff and also need updates?