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

Document the forceRebuild extension #89

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HighCommander4
Copy link
Contributor

No description provided.

@HighCommander4
Copy link
Contributor Author

@kadircet @hokein I don't have the permission to add you as reviewer in this repo, but could you take a look please?

@kadircet
Copy link
Member

Thanks for the patch, I feel like this isn't a "useful" extension nowadays as we've improved both picking up compile flags dynamically and also are keeping better track of preamble valid-ness.

Any specific motivation for the patch, apart from increasing docs coverage?

@sam-mccall were there any other historical use cases for this extension that might still be relevant? maybe we can just retire it going forward in clangd.

@HighCommander4
Copy link
Contributor Author

HighCommander4 commented Oct 18, 2023

Any specific motivation for the patch, apart from increasing docs coverage?

I believe this is being used by clangd users at Ericsson, with a modified vscode client.

The use case is a client-side action that creates a header file for which an #include was present (and unresolved due to the file not yet existing) in an open source file. Sending a forceRebuild causes clangd to re-check for the existence of the newly created file and remove the diagnostic.

Please do not remove the feature :)

@kadircet
Copy link
Member

The use case is a client-side action that creates a header file for which an #include was present (and unresolved due to the file not yet existing) in an open source file. Sending a forceRebuild causes clangd to re-check for the existence of the newly created file and remove the diagnostic.

Ok but I think that's exactly the improvement I mentioned above. If they were to send a didChange, even without forceRebuild set, clangd should still rebuild that preamble nowadays.

@HighCommander4
Copy link
Contributor Author

If they were to send a didChange, even without forceRebuild set, clangd should still rebuild that preamble nowadays.

Oh, interesting!

Let me follow up on this; will change the PR to draft for now.

@HighCommander4 HighCommander4 marked this pull request as draft October 20, 2023 07:44
Copy link
Member

@kadircet kadircet left a comment

Choose a reason for hiding this comment

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

argh, now i see why the confusion in the main thread occured. I wrote this comment but forgot to hit submit :( still getting used to the PR workflow..


Clangd makes an effort to invalidate ASTs when dependent files change, but
there are some changes it does not detect, for example when an unresolved
include becomes resolved because the header file has been created.
Copy link
Member

Choose a reason for hiding this comment

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

well that isn't strictly true nowadays, we actually do track missing include directives when building a preamble and will invalidate the preamble if any of them become available.

the only scenario that I can think of for forceBuild to be useful nowadays is, when foo.h becomes available in an earlier include search path than it was found before.

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.

2 participants