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

Shove VaList, et al. into ffi::va_list #401

Closed
workingjubilee opened this issue Jun 24, 2024 · 3 comments
Closed

Shove VaList, et al. into ffi::va_list #401

workingjubilee opened this issue Jun 24, 2024 · 3 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Jun 24, 2024

Proposal

That's the entire thing.

Problem statement

core/ffi/mod.rs feels like a bit of a mess1. Having the VaList impl in it is likely to make that worse over time if/when c_variadic gets stabilized, platforms get added, more functionality is considered, etc.

Motivating examples or use cases

The motivation is being able to read and understand libcore, and so we can delete the zillion redundant stability annotations in exchange for just one on the module.

Solution sketch

Move the types and traits into a submodule of ffi, for ffi::va_list.

Alternatives

  • We can add a reexport of the types themselves into ffi, so ffi::VaList is still accessible. One more stability annotation, but that seems okay.
  • We can not add such a reexport. One less stability annotation, which is fine by me.
  • We can name the module something else, like varargs, I guess.
  • We can accept chaos and stare endlessly into the abyss.

Links and related work

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

Footnotes

  1. This is surprising to me given that much larger modules have seemed fairly organized to me. I think it's all the cfg that makes me dizzy, plus the redundant annotations.

@workingjubilee workingjubilee added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jun 24, 2024
@joboet
Copy link
Member

joboet commented Jun 25, 2024

Given that the only public API is VaList and VaListImpl, I don't think that a separate public module is necessary. Separate modules are only really useful when you have lots of support items for a feature, like the iterators over os_str. We should instead just re-export these types from a private module, just like we do in the rest of std.

@workingjubilee
Copy link
Member Author

workingjubilee commented Jun 25, 2024

eh, it seemed worth considering while there is currently a minimum of entities there, instead of waiting until it happens, and because the API is niche so placing it out of the way seems fine.

there is currently-unimplemented support API for our emulation of the va_list type that could become added in the future, like the relevant GCC extensions which answer questions like "how do you forward a VaList to a ...", and I could see further Rust-grown extensions that use typestate hackery that is feasible in Rust and not in C to try to make it (slightly) safer for use with common patterns (like the somewhat omnipresent "so you want to reimplement printf").

I'm happy to make it just an internal organizational change though, if there's no appetite for this.

@joshtriplett
Copy link
Member

joshtriplett commented Jul 2, 2024

I do think we should re-export these types directly in core::ffi. 👍 for code organization, though.

(Having it be internal also sidesteps the question of va_list vs varargs vs ...)

@workingjubilee workingjubilee closed this as not planned Won't fix, can't repro, duplicate, stale Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

3 participants