Updated the patch to refactor mode code.

Remove some FIXMEs that are now done.

Change the approach: handle type merging for tag types using the ODRHash mechanism

I have to assume there was a point in the past where they were just one class, but it's been pretty confusing for a while. I think it's time to fix it.

Nice! LGTM

LGTM with some minor comment below!

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.

*using implicit modules in a build where compiler flags in different invocations aren't homogeneous, or something along those lines.

LGTM with one minor change. Can you add an entry in the modules docs for this flag and mention that using it can lead to more PCMs in an implicit build?

This approach looks overall much better! Unless @sammccall has any extra comments, it LGTM.

Hi Michael, thanks for working on this!

Ping!

Ping!

Did you try xxHash64?

Ping!

Update the patch to use two ::Fixed, 32 in abbrev to encode hash.

Remove pasto from one of the testcases

Update testcase to use a more portable version of touch

Nice! Are you planning to address the FIXME's in a later update of this patch?

LGTM!

Nice! Is this something that can be tested for in unittests/Basic/FileManagerTest.cpp?

Thanks for fixing this, LGTM!

I'm curious to know if you measured the impact on the PCM size after this change (example: PCMs from building macOS public SDK)? For instance, PPD_SKIPPED_RANGES currently explodes for some specific inputs, just wonder what's the story here.

LGTM with one minor change.

LGTM

Internal compiler error: LFS error for "/Users/jfb/s/llvmm/debug/tools/clang/test/Index/Output/pch-from-libclang.c.tmp.mcp/14EC4PG1UTVLY/modules.idx"failed to create unique file /Users/jfb/s/llvmm/debug/tools/clang/test/Index/Output/pch-from-libclang.c.tmp.mcp/14EC4PG1UTVLY/modules.idx.lock-0ae18733: No such file or directory

Thanks for working on this JF!

It seems conceptually a little strange to have the working directory be part of a serialized "FS", as it's fundamentally a property of a process and only transiently a property of the VFS.

Nice, LGTM!

Thanks for working on this. Can you add unit tests for those?

I am currently working on the next part of clang interface stubs that will take the interface stubs per compilation unit and merge them into one text stub (which will be used by something like llvm-elfabi to generate a stubbed out ELF .so file). I was using llvm-nm, but there are cases where the symbol is present in the .o file but it is marked as HIDDEN and llvm-readelf was what I was using to determine if the symbol was in fact marked as hidden. In these cases, the linker will not emit the symbol in the final .so file but it still needs the symbol as part of linking.

LGTM

Hi JF. Thanks for working on this, nice improvement to error handling!

ninja check-clang passes... is there anything else I should be testing?

Thanks for working on this, the approach & patch LGTM.

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

Nice, note that a subset of this information can be achieved with -Wauto-import, but this is way more general and solid, since it will handles @imports and pragma imports too. LGTM with a minor nitpick, see comments.

LGTM!

I missed that the DependencyCollector already marks them virtual, you are just making it obvious here. I think you can omit the ones that are already virtual and only add to the ones that are on the intend of this patch.

Not really. Would making only the attachTo* methods virtual enough though?

How much of the ModuleDependencyCollector will be reused as is by LLDB? I wonder about the tradeoff versus inheriting from DependencyCollector directly.

Thanks for working on this! LGTM