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

I like this. Can you start a discussion on cfe-dev (if you haven't already)? This is user-visible so I want to be sure other vendors are happy with this; if not, we can hide it behind a CMake flag.

I have a few nitpicks, but otherwise this seems right. I'll wait for the llvm-dev discussion before LGTM'ing though.

In D58548#1407355, @dexonsmith wrote:

In D58548#1407331, @greened wrote:

+1. Is there any reason not to use "%4" in the definition?

define i32 @f(i32, i32) {
  %3 = add i32 %0, %1
  br label %4

%4:
  ret i32 %3
}

Maybe it creates an ambiguity in the grammar or something.

A % isn't used for labels that do have names, so David's approach seems consistent.

In D58548#1407331, @greened wrote:

+1. Is there any reason not to use "%4" in the definition?

define i32 @f(i32, i32) {
  %3 = add i32 %0, %1
  br label %4

%4:
  ret i32 %3
}

Maybe it creates an ambiguity in the grammar or something.

In D20582#1407226, @arsenm wrote:

In D20582#1407184, @dexonsmith wrote:

It’s unfortunate that this will be quadratic in the number of modules. Is there a way we could improve that?

Well the linker API only exposes linking one module into one other at at time, so I don't know. Currently we have the AMDGPUUnifyMetadata pass as a workaround which cleans these up in a pass over the fully linked module, so that avoids revisiting for each module but it would make more sense if the linker dealt with this.

It’s unfortunate that this will be quadratic in the number of modules. Is there a way we could improve that?

In D58548#1407164, @dexonsmith wrote:

I like this idea, and I don’t think the textual IR central is too important.

I like this idea, and I don’t think the textual IR central is too important. A few things:

This seems like it could be useful. Do you have specific places in mind where you'd take advantage of it?

In D58384#1402418, @evgeny777 wrote:

Summary clusters, nodes and edges are retrieved from DenseMap with arbitrary order, which doesn't make result DOT file invalid.
Reverse iteration just exposes test issues.

I’d like @kubamracek and/or @Bigcheese to take a look first.

In D57688#1383598, @mclow.lists wrote:

Oh, I agree. However, when I have proposed this in the past, I have gotten pushback from many people (including Apple, Google and others).
At the very least, you should announce this on llvm-dev, and get feedback there.

In D56485#1383077, @marxin wrote:

Hi, there's no reply from the people who were added to this review.
May I understand that as ready to be installed?

In D56485#1369023, @marxin wrote:

I would suggest to revert the commit, and submit the new differential with properly subscribed lists this time.
At least that is the 'recommended' general practice.

Done.

In D56816#1360824, @smeenai wrote:

My understanding is that rL349841 accidentally started producing some spurious warnings/errors, rL350768 fixed some instances, and this change fixes more instances. Given that the first two changes are already in the 8.0 branch, should this be cherry-picked to 8.0 as well once it lands?

In D56802#1360316, @rjmccall wrote:

We've tossed around the idea of doing things like this before, but I was hoping that it wouldn't have to be specific to NSObject and that we'd e.g. have an attribute that guarantees that the @interface declares all the ivars for a class. Are we still thinking that?

The code looks good. Can you add a test too? Might need to require “shell”.

Note that a message send needs to have two expressions without an operator in between. Can we also rule out all square brackets that just have a single identifier or number token?

In D55869#1337706, @js wrote:

The ObjFW runtime itself does not contain anything about release, retain or autorelease - these are all part of ObjFW (as in the framework). As ObjFW also supports the Apple runtime, as well as mixing with Foundation code, it so far only provides objc_retain and objc_release when they are missing, to make ARC work. They just call into the corresponding methods on the object and do nothing else.

How will this change work from the Apple runtime? Will objc_retain/objc_release call the retain/release on the object if it implements its own? Should I drop my own retain/release implementation from my root object if I am compiling against the Apple runtime?

In D55869#1337692, @rjmccall wrote:

Okay. That's also presumably true for open-source runtimes that support ARC; tagging David Chisnall and Jonathan Schleifer to get their input.

In D53890#1335770, @pcc wrote:

This means that

clang -c -flto=thin foo.c
[upgrade clang]
clang -c -flto=thin bar.c
clang foo.o bar.o

will fail unless you add -fsplit-lto-unit to the second invocation. Given that the alternative is that we potentially silently miscompile, maybe that's fine. It would be possible to handle this case without aborting or miscompiling by splitting bar.o inside the linker and treating it as if it had always been split, but given that there's a workaround maybe that's not that critical.