Skip to content

use cfg_match! to select the right VaListImpl definition #141361

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

folkertdev
Copy link
Contributor

tracking issue: #44930

Just a bit of cleanup really.

We could use PhantomInvariantLifetime<'f> (#135806) to make it more precise what that PhantomData<&'f mut &'f c_void> marker is doing. I'm not sure how ready that feature is though, @jhpratt are these types good to use internally?


Some research into the lifetimes of VaList and VaListImpl:

It's easy to see why the lifetime of these types should not be extended, a VaList or VaListImpl escaping its function is a bad idea. I don't currently see why coercing the lifetime to a shorter lifetime is problematic though, but probably I just don't understand variance well enough to see it. The history does not provide much explanation:

Beyond that I don't see how the lifetime situation can be simplified significantly, e.g. this function really needs 'copy to be unconstrained.

/// Copies the `va_list` at the current location.
pub unsafe fn with_copy<F, R>(&self, f: F) -> R
where
    F: for<'copy> FnOnce(VaList<'copy, 'f>) -> R,
{
    let mut ap = self.clone();
    let ret = f(ap.as_va_list());
    // SAFETY: the caller must uphold the safety contract for `va_end`.
    unsafe {
        va_end(&mut ap);
    }
    ret
}

@rustbot label +F-c_variadic
r? @workingjubilee

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. F-c_variadic `#![feature(c_variadic)]` labels May 21, 2025
#[repr(transparent)]
#[derive(Debug)]
pub struct VaList<'a, 'f: 'a> {
inner: VaListImpl<'f>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this implementation that is used when we don't know the layout of va_list (the _ => { ... } case in the cfg_match! above). It is inconsistent with the others.

Could we not have &'a mut VaListImpl<'f> here like above, and make the VaListImpl a ZST containing just the invariance marker?

Is that fallback implementation even meaningful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok no, after reading a bit more there are just two approaches

  1. va_list is just an opaque pointer (probably just the next variadic argument on the stack, though it's opaque so we can't know)
  2. va_list is some sort of struct. It looks like this is used when registers are also at play for passing variadic arguments

These two approaches are captured by the two different ways of representing VaList. I added a comment with some of that information, in particular #56599 had some useful info.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does raise the question: what is VaList for? it has no methods of its own, so I don't see its purpose.

@folkertdev
Copy link
Contributor Author

blocked on #137198 for now, which renames cfg_match! tocfg_select!.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-c_variadic `#![feature(c_variadic)]` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants