Address review comments to #1306 #1309
Conversation
|
@swift-ci Please test |
Documentation/Files_To_Reindex.md
Outdated
| Affects all files that include the header (including via other headers). For existing headers, we know which files import a header from the existing index. For new header files, we assume that it hasn’t been included in any file yet and so we don’t re-index any file. If it is, it’s likely that the user will modify the file that includes the new header file shortly after, which will re-index it. | ||
| All files that include the header (including via other headers) might be affected by the change, similar to how all `.swift` files that import a module might be affected. Similar to modules, we choose to not re-index all files that include the header because of the same considerations mentioned above. | ||
|
|
||
| To re-index the header, we pick one main file that includes the header and re-index that, which will effectively update the index for the header. For existing headers, we know which files import a header from the existing index. For new header files, we assume that it hasn’t been included in any file yet and thus don't re-index it. If the header is included from somewhere lese, it’s likely that the user will modify the file that includes the new header file shortly after, which will index the header and establish the header to main file connection. |
There was a problem hiding this comment.
For new header files, we assume that it hasn’t been included in any file yet and thus don't re-index it
re-index -> index given we haven't actually indexed it yet?
If the header is included from somewhere lese
lese -> else. But also, is the case you're referring to here if it's no longer included in the original file?
There was a problem hiding this comment.
lese -> else. But also, is the case you're referring to here if it's no longer included in the original file?
Changed it to
If the user wrote an include to the new header before creating the header itself, we don't know about that include from the existing index.
| @@ -350,21 +352,21 @@ public final actor SemanticIndexManager { | |||
| switch newState { | |||
| case .executing: | |||
| for file in files { | |||
| if case .scheduled((taskID, let task)) = self.indexStatus[file.uri] { | |||
| self.indexStatus[file.uri] = .executing((taskID, task)) | |||
| if case .scheduled((taskID, let task)) = self.indexStatus[file.file.sourceFile] { | |||
There was a problem hiding this comment.
file.file.sourceFile is... amusing 😅. I don't have a great suggestion here outside of naming sourceFile -> sourceUri and then maybe the variable file -> fileWithTarget. Or just leave them but add a let sourceFile = file.file.sourceFile so we don't have the many references 🤷
There was a problem hiding this comment.
Changed it to fileAndTarget.file.sourceFile. It’s at least a little better.
ahoppen
force-pushed
the
review-comments-1306
branch
from
9345a4f
to
0202391
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
Essentially fix two issues in updating the index store: 1. If there was one task to index `HeaderA.h` through `main.c` and one to index `HeaderB.h` through `main.c`, we would not declare a dependency between them in the task scheduler, which meant that we could have two concurrent and racing index tasks for `main.c`. Declare a dependency between any two files that have the same main file 2. `UpdateIndexStoreTaskDescription` was computing the target to index a file in independently of `SemanticIndexManager`. While they currently always line up, we should pass the target in which to index a file to the `UpdateIndexStoreTaskDescription`. Only this way can we guarantee that we actually prepared the target that the file will be indexed in.
ahoppen
force-pushed
the
review-comments-1306
branch
from
0202391
to
6695859
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
1 similar comment
|
@swift-ci Please test Windows |
A few minor fixes and the following bigger change
Improve handling of main file vs header file during indexing
Essentially fix two issues in updating the index store:
HeaderA.hthroughmain.cand one to indexHeaderB.hthroughmain.c, we would not declare a dependency between them in the task scheduler, which meant that we could have two concurrent and racing index tasks formain.c. Declare a dependency between any two files that have the same main fileUpdateIndexStoreTaskDescriptionwas computing the target to index a file in independently ofSemanticIndexManager. While they currently always line up, we should pass the target in which to index a file to theUpdateIndexStoreTaskDescription. Only this way can we guarantee that we actually prepared the target that the file will be indexed in