-
Notifications
You must be signed in to change notification settings - Fork 58
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
clangd stop in "parsing includes, running Build AST" for long time becase of bugprone-unchecked-optional-access #1700
Comments
Speaking of |
I don't think this is reasonable based on my limited knowledge of clangd and clang-tidy.
However, I think it would be nice if there's an alternative to write the progress in the log for debugging purposes, if possible. |
just for error check, In most case when we have many check, clang-tidy may be slow, and when clangd stop by clang-tidy, highlight is not work. make the situation confusing. As I expect, clangd should work fine with highlight when process clang-tidy (maybe with out diagnostic) |
Yeah. But if we do emit detailed progress for each check, chances are that users would see text flickering in the status bar, which IMO is not a good UX design. (And I guess updating status rapidly might bring some harmful performance impact to editors.)
IIUC, language features like highlighting requires the AST for a .cpp file. Under the current translation unit scheduling model, we're running the clang-tidy in the AST building thread. Clang-tidy requires the AST built by clangd, which would surface the diagnostics from clang-tidy later.
If we separate the clang-tidy from the AST thread, we'll need a synchronization mechanism to merge diagnostics from e.g., include-cleaner, clang-tidy, and clang. I don't think this is trivial to implement. |
Sounds like we should disable unchecked-optional-access as a slow check. In general dataflow-based checks are going to be slow sometimes, we should probably disable them until there's a real plan. We could send a status update for "running tidy checks" potentially. We can't really indicate which, because their execution is interleaved: a single AST traversal reports matches for all checks as it finds them. The flicker issue could be addressed by a denounce on the client or server, though we don't really have the thread infra to do it conveniently on the server and doing it on the client leaves the protocol chatty... |
Agreed, a number of clangd users have run into issues with this checker (not just slowness but also crashes). I do wonder if, before we add more checks to the list which are force-disabled as "unusable", we should first add the override mechanism discussed in #1337 (comment)? We have a contributor interested in implementing that mechanism, they're just waiting for a signal that it would be accepted -- @sam-mccall could you kindly weigh in on that? |
Can confirm I am having the same issue that was resolved by disabling Working with Vulkan SDK, I thought my issue was the enormous generated headers (e.g., I certainly don't have intimate knowledge of clangd, but I know a lot of users are going to be like me and copy a basic I would definitely appreciate some obvious (but opt-in) way to protect myself from my own ignorance :)
Just spitballing with these suggestions, no idea on feasibility or if they're helpful. Edit: my input above is conflating clangd with clang-tidy, which is not helpful. Sorry about that. |
Fixes llvm/llvm-project#69369. Fixes clangd/clangd#1700. (cherry picked from commit e63ab13c82e78f65baca48d5b5e4f6ea8d55dbc7)
Fixes llvm/llvm-project#69369. Fixes clangd/clangd#1700. (cherry picked from commit e63ab13c82e78f65baca48d5b5e4f6ea8d55dbc7)
I found that Clangd often gets stuck in the 'parsing includes, running Build AST', and after using Mac's profile tool, I found that the actual stack is as follows
after disable
bugprone-unchecked-optional-access
in clangd configuration, every thing be ok.I wonder if we can separately prompt for 'runing clang-tidy check xxxxx' instead of displaying 'parsing includes, running Build AST'
The text was updated successfully, but these errors were encountered: