LGTM

remove header_file_size()

In D143414#4110461, @benlangmuir wrote:

This should allow the path serialization of input files to use the paths used when looking up a file entry, instead of the last reference.

Isn't this at odds with not having the VFS-mapped paths?

It's not obvious to me why we want these specific semantics. Elsewhere we have tried to preserve the virtual paths as well as the vfsoverlay files needed to interpret them. Is there a reason the current approach is better? I feel like there may be context here I'm lacking

Use FileEntryRef, rename method

Don't return virtual file entries

Sort by UIDs, then Name

In D142780#4087273, @jansvoboda11 wrote:

Sounds like the easies way out is serializing all FileEntryRef objects we know in deterministic order.

In D142780#4087270, @benlangmuir wrote:

If compiling a single pcm accesses multiple hard links with the same UID, then it would not be possible to use the set of UIDs to get the "right path". At best we could make it get a deterministic path -- e.g. if we tracked the order of access.

In D142780#4087236, @benlangmuir wrote:

I think if we want to change this to FileEntryRef it needs to be deterministic which ref you get.

Use FileEntryRef for AddTopHeader()

rebase on last successful build: https://buildkite.com/llvm-project/llvm-main/builds/6356

address comments

Its going to be less work to refactor TopheaderNames to use FileEntryRef, so went that way instead. First step is here: https://reviews.llvm.org/D142113

In D142028#4062798, @akyrtzi wrote:

Is it reasonable to at least have an alternative implementation for it (e.g. from the module headers find the headers that were not included from other headers), not sure if it'd be straightforward or not.

@akyrtzi it looks like you added this field a while back. Do you know if there are still consumers for the the libclang API, do we need to keep this field around?

In D133586#3831618, @vsapsai wrote:

How correct is it to access isConst, isVolatile, isRestrict for FunctionNoProtoType? Yes, we can provide some default value but I'm curious if accessing that default value is correct.

For the record, I've tried to fix the same problem in https://reviews.llvm.org/D104963 in a different way.

Nice, LGTM, thanks!

remove else case

Remove unnecessary friend class

Return default Qualifiers for FunctionNoProtoType

add -std=c89 to test

In D133586#3809475, @aaron.ballman wrote:

This is why I'm wondering how we're hitting this problem in the first place. C++ shouldn't be able to create a function without a prototype so why does the ODR hash matter (do we use that in C and I just wasn't aware of it)?

rebase

Specify target and language for test

Add unit test for type qualifiers

In D133586#3781536, @rtrieu wrote:

In D133586#3781468, @rmaz wrote:

In D133586#3781381, @rtrieu wrote:

Do you have a test case for this?

Was struggling to think of a good one. How about a test that repeatedly generates a pcm for a func decl with no params and checks if the DECL_FUNCTION record is the same?

Have you looked at clang/test/Modules/odr_hash.cpp? It's where most of the ODR hash testing takes place by testing that Decls can be merged properly instead of checking the contents of pcm files.. Using #if define, it creates multiple modules from the same file. I would suggest creating two functions in each of the modules, then in the main file, using the function to force it to be loaded from the modules and merged together. The test should fail with the current Clang, but pass with your patch. You may need to create your test file if you need different compiler options.

zero out qual types in constructor

I think it is probably safer to zero out the FastTypeQuals instead.

clang-format

rebase

In D133586#3781381, @rtrieu wrote:

Do you have a test case for this?

Move code to VisitFunctionProtoType instead of branch

I see the producers of the sections here:

• https://github.com/llvm/llvm-project/blob/main/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L4910-L4938
• https://github.com/apple/swift/blob/91b2abe99686600cd18daa2e898d8376440fba9e/lib/IRGen/IRGen.cpp#L1131-L1140
• https://github.com/llvm/llvm-project/blob/main/clang/tools/driver/cc1as_main.cpp#L517-L524

In D130710#3688338, @akyrtzi wrote:

In D130710#3685436, @benlangmuir wrote:

Is the functionality provided by ORIGINAL_PCH_DIR still useful for making it possible to move a PCH and its referenced headers? It's not completely clear to me when this feature is used in practice. It would be nice to remove it or change the default behaviour if possible, rather than require a new option, but I'm open to this approach if we think we can't change the default.

Given that there was a recent change related to ORIGINAL_PCH_DIR, I'm reluctant to change the default at this point, I added a FIXME for following up to see if we can remove it or change the default later on.

In D124946#3500454, @urnathan wrote:

Looks good to me, but perhaps leave it a few days for others to comment (my familiarity with this code is low). I do know people want relocatable outputs though.

Refactor branch into existing case

fix for missing temp dir in test

fix windows paths attempt #2

Use regex for path separators in test

With linter

Remove unnecessary CHECK-NOT

Use llvm-readobj --macho-segment

Single pass tests

update test for windows

with linter