Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cxx_extractor): the main source file should be marked always_process #5537

Merged
merged 2 commits into from Mar 14, 2023

Conversation

shahms
Copy link
Contributor

@shahms shahms commented Mar 14, 2023

There is some argument to be made that this logic should (also?) go in the indexer as the indexer should always process the primary source file, regardless of claiming, but we already have a facility for this in the extractor and just need to change one place so that the primary source file is always processed.

Background: when using dynamic claiming, prospective claims are issued with a timeout, but if a given compilation unit abandoned midway through indexing it can sometimes be retried before the previous prospective claims have expired, thus skipping files which it should index. While this is problematic for header files as well, it is most troublesome for source files which are unlikely to be indexed at all as a result. Header files may potentially be included by another compilation unit and therefore indexed separately.

Pros of this approach: very small change, uses pre-existing facility.
Cons:

  • This arguably belongs in the indexer itself
  • can't be flag-controlled in extractor
  • need to update all of the tests due to the change in the resulting proto files

@shahms shahms requested review from zrlk, danielmoy-google and a team March 14, 2023 21:43
Copy link
Contributor

@jaysachs jaysachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree it belongs in the indexer but this is terribly convenient.

@shahms shahms merged commit ad0966b into kythe:master Mar 14, 2023
@shahms shahms deleted the cxx-extractor-always-process-main-file branch March 14, 2023 21:59
@creachadair
Copy link
Contributor

This seems like a nice workaround. Our original idea for static claiming was too expensive to be practical on google3, but this is a decent tradeoff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants