-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix clangd for JIT files #20741
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
base: master
Are you sure you want to change the base?
Fix clangd for JIT files #20741
Conversation
| #include "zend_vm_opcodes.h" | ||
|
|
||
| #include "jit/zend_jit.h" | ||
| #include "jit/ir/ir.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IR related dependencies should be encapsulated in zend_jit_ir.h.
I know, this is not completely true now, but I wouldn't spread dependency even more.
dstogov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand the reason for this permutation.
This doesn't make sources better at all, but would complicate merging between major branches.
I would understand making a better decomposition of JIT to separately compiled C files with minimized cross-dependencies. This is a bit bigger task, but it would beneficial for humans and not only for clangd.
|
My assumption was that the C files are included directly for better cross-file optimizations. I'm happy to try to split them comletely, though that won't exactly result in a smaller change or help with merge conflicts. It will add a huge header file for helpers with a silent conflict (no merge conflict, but if the signature of a helper changes, it will need to be updated in the header file in the master branch). Clangd is pretty essential for me to work in a codebase, especially one I don't know by heart. I don't care if it's this approach or if we can find something else, but it would be great to fix somehow. |
|
I also use clangd, and it does help a lot for code nagivation (more than ctags), finding usages of a function, real time checking/linting, etc. This change would reduce friction when contribution to the JIT, which is a good thing.
This would be great, but as @iluuu1994 noted there is likely some performance impact. Some of which could be mitigated by moving some function definitions to headers. The proposed change wouldn't preclude doing this ultimately. If we split the new header file into |
No description provided.