[clang Dependency Scanning] Out-of-Date Scanning File System Cache Entry Reporting C-APIs #10927
Conversation
qiongsiwu
requested review from
Bigcheese,
jansvoboda11,
owenv and
cachemeifyoucan
|
Note to reviewers: I don't see a good way to cleanly test these APIs through c-index tests. Ideally, I'd like to create an underlying service whose file system cache I can manipulate directly to test these APIs directly (something similar to https://github.com/llvm/llvm-project/pull/144105/files#diff-25ec4b41a8aed83f312f2bb7b409f379f38dd9d1597d6e4e45e17681260dfcc2R205). Do we think it is reasonable to create unit tests for these C-APIs? If so, I will try making some unit tests. |
…cative and concise.
|
@swift-ci please test llvm. |
|
@swift-ci please test llvm. |
|
The two failing tests do not relate to this PR. rdar://154865941 is tracking them. |
|
Gentle ping for review. Thanks! |
clang/unittests/libclang/DependencyScanningFSCacheOutOfDateTests.cpp
Outdated
Show resolved
Hide resolved
|
Adding the unit test is a nice bonus, but I'd be in favor in keeping the old |
This is a good point. The difficulty is that it is not easy to create out-date-entries exactly as we desired to make sure we have good coverage of all the APIs. I can recover the removed |
|
@swift-ci please test llvm. |
clang/unittests/libclang/DependencyScanningFSCacheOutOfDateTests.cpp
Outdated
Show resolved
Hide resolved
clang/unittests/libclang/DependencyScanningFSCacheOutOfDateTests.cpp
Outdated
Show resolved
Hide resolved
clang/unittests/libclang/DependencyScanningFSCacheOutOfDateTests.cpp
Outdated
Show resolved
Hide resolved
|
@swift-ci please test llvm. |
…ook at directories for size changes.
|
@swift-ci please test llvm. |
| @@ -131,7 +131,11 @@ DependencyScanningFilesystemSharedCache::getOutOfDateEntries( | |||
| // later. The cache entry is not invalidated (as we have no good | |||
| // way to do it now), which may lead to missing file build errors. | |||
| InvalidDiagInfo.emplace_back(Path.data()); | |||
| } else { | |||
| } else if (Status->getType() == | |||
There was a problem hiding this comment.
Hmm, this ignores symlinks that now point to path of different size. I don't think it's crucial to add support for those right now, but maybe it's worth pointing out in the comment below.
| @@ -131,7 +131,11 @@ DependencyScanningFilesystemSharedCache::getOutOfDateEntries( | |||
| // later. The cache entry is not invalidated (as we have no good | |||
| // way to do it now), which may lead to missing file build errors. | |||
| InvalidDiagInfo.emplace_back(Path.data()); | |||
| } else { | |||
| } else if (Status->getType() == | |||
There was a problem hiding this comment.
This also makes me think of edge-cases where the type of the FS entry changed. Can we make sure we're comparing apples to apples (files to files) by checking Entry->getStatus()->getType() too? (Maybe we should explicitly report entries that changed type, but let's not block this PR on that.)
There was a problem hiding this comment.
I think changes here should also go upstream, right? (It's fine to do as a follow-up to reduce churn and waiting.)
|
|
||
| EXPECT_TRUE(CheckedNegativelyCached && CheckedSizeChanged); | ||
|
|
||
| clang_experimental_DepScanFSCacheOutOfDateEntrySet_disposeSet(Entries); |
There was a problem hiding this comment.
This should probably dispose of the other objects as well.
This PR implements the C-APIs to report a scanning file system cache's out-of-date entries. The C-APIs contains a function to return a set of file system cache out-of-date entries, functions to facilitate looping through all the entries, and reporting the relevant information from the entries.
The APIs are based on llvm#144105.
rdar://152247357