In D150397#4574504, @phosek wrote:

In D150397#4574476, @ahatanak wrote:

We are currently investigating this issue.

Thanks for taking a look!

Are you (Flutter) using your custom toolchain based on ToT instead of the current shipping Xcode toolchain to build the app?

Yes, we use our Clang toolchain that tracks tip-of-tree.

LGTM

Thanks for the patch!

Hi Usman! This is looking very good and complete now! Thanks for taking the time to learn and fix this.

In D146759#4225656, @Unique_Usman wrote:

Thanks for your reply sir. Regarding the comment, I would remove it sir.

Sir, about updating/adding the test cases, I have never written any test case before sir, kindly guide me through that sir?

Please update/add test cases.

LGTM for the ExtractAPIVisitor part.
Remaining:

• update test with @LINE
• the libclang side

In D146759#4217882, @Unique_Usman wrote:

Also, if there is a manual/README.md file to better understand the DeclarationFragments.cpp/ExtractAPI, kindly point it out so, I can go through it, thank you.

Thanks for creating the review!

Good to see refactoring to make ExtractAPI easier to extend and use. Thanks Daniel!
Some comments on the ExtractAPIVisitor part. I'll leave the libclang part to the experts.

In D145206#4167149, @tmatheson wrote:

I'm not sure that this is the right fix. The exception indicates that this is being called with ArchInfo objects with invalid VersionTuples, how is that happening? Also could you explain what the issue is with bad_optional_access on older macOS?

This fixes the checking logic, and also works around a build failure with clang/unittest/Interpreter/ExceptionTests which enables exceptions. std::optional::value() throws std::bad_optional_access, which won't build if targeting older versions of macOS

Hi! This broke the asan replaceable_new_delete.cpp test on Darwin because it has a run line using -static-libsan. Could you take a look? Probably need to separate that check out and mark as unsupported on Darwin

Is it easy to/worth checking if the target is actually outside of the current module to keep the output smaller and concise?

In D135295#3837895, @zixuw wrote:

In D135295#3837855, @zixuw wrote:

In D135295#3837734, @ributzka wrote:

This doesn't affect any tests?

Just finished building locally, running tests to verify now

Doesn't affect any test for this patch as expected. Still need to see how does it interacts with https://reviews.llvm.org/D134813.

With the PrintingPolicy fix in https://reviews.llvm.org/D135295 and landed USR fix, the diff within ExtractAPI tests is only the wording with anonymous enums, and we can drop the lit change:

658c658
<             "spelling": "(anonymous)"
---
>             "spelling": "enum (unnamed)"
661c661
<         "title": "(anonymous)"
---
>         "title": "enum (unnamed)"
664c664
<         "(anonymous)"
---
>         "enum (unnamed)"
706c706
<         "(anonymous)",
---
>         "enum (unnamed)",
746c746
<             "spelling": "(anonymous)"
---
>             "spelling": "enum (unnamed)"
749c749
<         "title": "(anonymous)"
---
>         "title": "enum (unnamed)"
752c752
<         "(anonymous)"
---
>         "enum (unnamed)"
794c794
<         "(anonymous)",
---
>         "enum (unnamed)",

@dang @QuietMisdreavus for this change.

I'm pulling this on top of https://reviews.llvm.org/D135295 to try locally now.

In D135295#3837855, @zixuw wrote:

In D135295#3837734, @ributzka wrote:

This doesn't affect any tests?

Just finished building locally, running tests to verify now

Update on top of the existing PrintingPolicy in the ASTContext instead of creating a new default one.

In D135295#3837734, @ributzka wrote:

This doesn't affect any tests?

Set PrintingPolicy properly

Setting PrintingPolicy::AnonymousTagLocations to false in https://reviews.llvm.org/D135295

Ah sorry I just finished reading the discussions. IIUC 20c9ac29250493f5e0a3791dc1e5e9114ff0dc6e should have already fixed the USR generation part, and all of the USR updates in the test cases should be gone now?

In D134813#3836836, @aaron.ballman wrote:

I'd guess we need some kind of change to CommentXML and ExtractAPI, but I don't know enough to be sure what it should be.

Ping @dang @zixuw and @dexonsmith for questions about how to handle ExtractAPI changes, and @gribozavr for questions about CommentXML

LGTM!

Thanks Daniel!

Awesome! Thanks Daniel!

LGTM. Thanks Daniel!

In D125678#3517168, @QuietMisdreavus wrote:

clang-format failed:

---  clang-format

changed files:

    clang/test/ExtractAPI/underscored.c

Well, apparently there are still some corner-case bugs in the logic. ExtractAPI and a few other tests failed. Will look into it tomorrow.

In D124964#3492423, @dang wrote:

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?

Whitespace change: clang-format removed an empty line

Accidentally added another test case in my local workspace. Removed in 5f841c71fc2cc77c92f526791cd7a938bcac69aa

Update test case to use cc1 instead of driver

In D123831#3490684, @dang wrote:

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

• Capture whether the include is quoted in KnownFiles
• Misc: use getInputBufferName() instead of string literal

Convert file path to use slashes for headermap reverse lookup

• Delete test known_files_only_hmap
• Handle quoted includes
• Attempt to fix Windows fails by converting backslashes before matching the framework regex
• Update test relative_include

LGTM!

• Rewrite commit message for preparation
• Remove shortening paths based on the current working directory: it does not work with angled includes, and unnecessary for our use
• XFAIL test known_files_only_hmap.c, as it is not a valid setup with a questionable headermap. Need to determine how to fix that test case or just discard it, as the new relative_include.m test also checks that symbols from external files are dropped

• Create FileManager in PrepareToExecuteAction
• Use FileManager to load headermaps and reverse lookup mappings
• Use FileSystem to correctly get working directory and make absolute paths

The problem is that we are trying to shorten the input file paths in PrepareToExecuteAction, where the CompilerInstance is still primal and doesn't even have a FileManager that we could use. That makes it hard (if possible at all) to reverse lookup headermaps and use the spelled names if we failed to find a search path prefix.
The impact is that we won't be able to get an angled include name for cases where an input doesn't reside in any of the normal search paths, but a headermap entry maps some name to that path. Or we might get the wrong include name where there are both headermap entries and search paths that match the input path and the ordering matters.

• Use first match in getRelativeIncludeName
• Try to get Preprocessor and reverse lookup headermaps in getRelativeIncludeName
• Use getRelativeIncludeName to check for known files in LocationFileChecker
• Misc fixes & adjustments

Add test case to demonstrate the framework case.

LGTM