In D125678#3517174, @zixuw wrote:

In D125678#3517168, @QuietMisdreavus wrote:

clang-format failed:

---  clang-format

changed files:

    clang/test/ExtractAPI/underscored.c

I think it's fine. clang-format always get tripped on split-file tests because of the mixed format and what not.

LGTM! If I understand the issue correctly we gave llvm::Regex::match a string temporary to match against before and now we store it for long enough to process the match results?

Since this is a new test can we use the approach in https://reviews.llvm.org/D124634 to check for diagnostics output.

LGTM

Minor comment LGTM otherwise

LGTM!

In D123831#3459368, @ributzka wrote:

In D123831#3458774, @dang wrote:

In D123831#3455048, @cishida wrote:

we might not always want to transform an absolute path because the resulting relative include name might get remapped in a headermap, for example in test known_files_only_hmap.c. But how does it work with modules where we need relative includes? Is the setup in known_files_only_hmap even valid?

I think, in most cases, this shouldn't matter because if the header path input doesn't match the location stored in the header map, they should still have the same source content. The same should be true with header search resolution with modules & vfsoverlay

Agreed, I think it would be classified as a user error to remap to different source content via headermap.

This is happening today in Xcode. Headers that are mapped from the DSTROOT back to the SRCROOT can be different, because they are not simply copied. Xcode also runs unifdef on them.

In D123831#3455048, @cishida wrote:

we might not always want to transform an absolute path because the resulting relative include name might get remapped in a headermap, for example in test known_files_only_hmap.c. But how does it work with modules where we need relative includes? Is the setup in known_files_only_hmap even valid?

I think, in most cases, this shouldn't matter because if the header path input doesn't match the location stored in the header map, they should still have the same source content. The same should be true with header search resolution with modules & vfsoverlay

Add a second anonymous enum to the test case to make sure that distinct symbols
are generated for both of them.

Address code review feedback:

Address code review feedback.

Add test to check that ObjC methods with multiple external parameters get the
correct name in subHeadings and navigator.

LGTM

Rebase on top of latest changes.

LGTM thanks for spotting this!

Rebase on top of latest changes.

Remove unnecessary include.

Make SymbolGraphSerializer::serializeAPIRecord const again.

Address code review feedback:

Address code review feedback.

Manage PatchComponents stack manipulation using RAII to avoid forgetting to pop
the stack when returning early from serializeAPIRecord.

Add descriptive method names for manipulating the path component stack.

Address code review feedback:

Whitespace changes

In D123045#3428030, @QuietMisdreavus wrote:

In D123045#3427992, @zixuw wrote:

In D123045#3427699, @QuietMisdreavus wrote:

After a quick scan comparing the current output of these symbol graphs with the primary library used for reading them, the last thing i can spot that's "off" is that the "function signature" is currently being serialized under a parameters field instead of the required functionSignature.

Hmm, I was looking at the format specification at https://github.com/apple/swift-docc-symbolkit/blob/0a45209833f4a151212c1aa38e13cfc03b9462e4/openapi.yaml#L307, and I searched the term functionSignature in the spec but found no property with that name (except for the FunctionSignature schema that the parameters property is referring to). But anyway this should be a easy fix.

It seems like the specification and implementation have diverged. The parser in swift-docc-symbolkit is looking for a functionSignature field by virtue of how it names its "coding key". By comparison, here is the functionSignature emitter in the Swift symbol-graph generator.

LGTM!

In D122798#3427004, @zixuw wrote:

Has this landed yet?

Doing it now!

529a0570f7e8c5144bd3ad057e43f00e3af58d1b

a9909d23e9bb8c4649cba1c14d479c28df4ca185

@thakis I am UK based so I am logging off as it's 9pm over here. The fix I committed has fixed the issue on other buildbots that were failing due to this. If it still fails on Windows, would you be able to revert the change for me?

Committed a revert of the changes in that particular function in commit 985eaa1a3da2a1b88ea70a65ffd5783aa82ea65e

I have what should be a NFC fix that should fix the issue, just checking everything still works and will commit it.

Update with code review feedback and the rebased code.

Update using the correct patch this time around.

Rebase on top of latest changes

Address review feedback.

LGTM!

Some errors slipped by when splitting this out of the macros patch.