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

[RFC] Let the compiler do inlining #156

Open
ollie-etl opened this issue May 17, 2021 · 4 comments
Open

[RFC] Let the compiler do inlining #156

ollie-etl opened this issue May 17, 2021 · 4 comments
Labels

Comments

@ollie-etl
Copy link

In several places in the code here, #defines are used to declare functions. Is there a rational for this?

The drawbacks of this style are it:

  • removes any semblance of compiler checked type safety,
  • makes generating FFI bindings to this library unnecessarily manual (and more error prone). This is certainly true for rust, and probably for python, although I haven't tried that recently.
  • reduces IDE tooling support.

I can't think of a rational for using this style - other than maybe you don't trust a compiler to inline - although that seems a pretty low bar.

I'm willing to submit PR(s) to address this, if desired

A related issue would be using consts in place of #defines for constants. The rational is similar, you gain type safety, tooling support, and simplify FFI generation.

@ollie-etl ollie-etl changed the title Let the compiler do inlining [RFC] Let the compiler do inlining May 17, 2021
@edmooring
Copy link
Contributor

edmooring commented May 18, 2021

These are all good points. I don't know of any reasonable argument against them. PRs will be welcomed. @arnopo, any comments?

@arnopo
Copy link
Contributor

arnopo commented May 18, 2021

Yes your proposal is certainly valid and a PR would help to estimate the impacts.
For now, my only concern is the footprint of libmetal due to the replacement of some #define by constant variables...
So please separate the functions and constants updates into two separate patches.

@ollie-etl
Copy link
Author

I'm happy to separate those concerns. I've created #158 to record that

@github-actions
Copy link

This issue has been marked as a stale issue because it has been open (more than) 45 days with no activity.

@github-actions github-actions bot added the Stale label Oct 15, 2023
@arnopo arnopo reopened this Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants