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

Limitation of the current extension system. #356

Open
br1tney5pear5 opened this issue Jan 1, 2025 · 2 comments
Open

Limitation of the current extension system. #356

br1tney5pear5 opened this issue Jan 1, 2025 · 2 comments

Comments

@br1tney5pear5
Copy link
Contributor

br1tney5pear5 commented Jan 1, 2025

Hi,

I'm raising this because I think the current extension system is limited and especially looking into the future as the code grows it could be improved a lot. Please let me know your thoughts on this.

Problem

My primary concern revolves around the zsv_cb struct passed to extensions. While I can understand the reasoning behind this approach when designing an extension system, I believe it is flawed.

First, adding new callbacks is unnecessarily arduous, often requiring the creation of wrapper functions just to allow extensions to access functionality that’s already exposed via the main API. A more streamlined approach would allow functions to be marked as exported, so extensions can use them directly.

In the proposed approach, the same headers would be shared between the core code and extensions without duplication—potentially with a subset made available specifically to extensions. This would address a common issue where extension is unaware of the internals of a struct it needs to use and either headers have to moved around or numerous getters added to callbacks.

For instance, with the subsystems I've added—procedures, key bindings, and context menu—extensions will need to interact with them in the same way as the core code. As such, extensions should have access to all the same methods and structs. Duplicating all the APIs just for extensions feels like a design flaw.

In addition to these usability concerns, there’s a more practical issue: changing the struct, such as adding or removing an entry in the middle of the struct, breaks compatibility with previously compiled extensions. Ideally, extensions compiled before such change should still work with a version of zsvsheet compiled after, and vice versa. Requiring both to always be rebuilt together defeats part of the purpose of having an extension system in the first place.

Proposed solution

A more effective approach would be to look at how the Linux kernel handles extensions. The kernel doesn’t rely on a vast array of callbacks passed to modules. Instead, it marks functions with EXPORT_SYMBOL if they are intended to be used by external modules.

The kernel performs its own symbol resolution, but we could adopt a similar strategy by leveraging the dynamic linker. By default symbols in executables aren't dynamically resolvable but can be made so by using something like --export-dynamic [1] linker flag when building the binary. This flag adds all symbols to the dynamic section of zsvsheet binary, allowing extensions to call them just as the core code would. If exposing all symbols is undesirable, --dynamic-list [1] could be used to specify a list of selected symbols to export. A macro could be used for this purpose, such as:

#define EXPORT_SYMBOL(NAME) enum {} /* do nothing, just force a semicolon */
int foo();
EXPORT_SYMBOL(foo);

Then, during the build process, we could parse all files for exported symbols and generate a dynamic list for the linker. This approach would address the issues mentioned above and significantly simplify the codebase. With each pull request I submit, the next task tends to be "make this new feature available to extensions," and I believe it should be as simple as marking the relevant functions with EXPORT_SYMBOL.

Let me know your thoughts. If you don't think this is viable feel free close the issue. Thanks.

References

  1. https://sourceware.org/binutils/docs/ld/Options.html
@liquidaty
Copy link
Owner

Agreed this would be a better approach. As a start, how about you use this approach for any of your current work that would otherwise require you to add functions to zsv_cb? We can then phase out zsv_cb in stages.

@br1tney5pear5
Copy link
Contributor Author

I'm glad to see you're on board with the idea. I also agree that the transition should be gradual.

As I mentioned earlier, the new system will require a script to run before the build. Since I'm not very familiar with autotools, I would appreciate any guidance on where this might be integrated.

On a broader note, is there a specific reason autotools was chosen, or was it simply the decision made at the start of the project? I've noticed a few issues with the current build setup—for instance, the zsvsheet is being unity built by directly including .c files in sheet.c. When someone gets a chance to address this, would you be open to considering a switch to something more contemporary and maintainable like CMake?

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

No branches or pull requests

2 participants