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

[support.runtime] Clarify va_start requirements #703

Closed
wants to merge 1 commit into from

Conversation

AaronBallman
Copy link
Contributor

"Compatible" is a bit of an ambiguous phrase, use "the same" instead, as that is the intention of the wording.

… same' instead of the more ambiguous term 'compatible'
@@ -3378,7 +3378,7 @@
\tcode{parmN}.}
If the parameter
\tcode{parmN}
is of a reference type, or of a type that is not compatible with the
is of a reference type, or of a type that is not the same as the
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, on reflection, I think "the same" is probably too strict. C allows cv qualifiers to differ, and IIRC also allows signed/unsigned mismatches. Maybe this is too large to fix editorially.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zygoloid: The signed/unsigned relaxation applies to the arguments passed to the ellipsis. By contrast, the text in question applies to the argument for the named parameter paramN.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, the signed/unsigned relaxation is a separate rule that's applied to arguments that don't match declared parameters (either because there's no prototype, or because the prototype ends in an ellipsis), and is not part of the notion of "compatible".

@AaronBallman
Copy link
Contributor Author

C allows cv qualifiers to differ,

I don't believe this is the case. 6.2.7 says two types are compatible if they're the same, otherwise see elsewhere. 6.7.3 is the elsewhere for type qualifiers and says (in p10): "For two qualified types to be compatible, both shall have the identically qualified version of a compatible type; the order of type qualifiers within a list of specifiers or qualifiers does not affect the specified type."

and IIRC also allows signed/unsigned mismatches.

I also don't think this is correct. 6.2.7 leads us to 6.7.2 for type specifiers where it shows signed/unsigned are not the same type.

I checked with David Keaton from WG14 and he agreed with my analysis of the C standard. Do you think this is still too large to fix editorially?

@zygoloid
Copy link
Member

zygoloid commented Jun 8, 2016

@tkoeppe Thanks, you're right -- I was thinking of this rule (from 7.16.1.1/2):

If there is no actual next argument, or if type is not compatible with the type of the actual next argument (as promoted according to the default argument promotions), the behavior is undefined, except for the following cases:

  • one type is a signed integer type, the other type is the corresponding unsigned integer type, and the value is representable in both types;
  • one type is pointer to void and the other is a pointer to a character type.

... but we're talking about va_start not va_arg here so this is irrelevant.

Taking a step back, the current wording simply makes no sense. How can a parameter be "of a type that is the same as the type that results when passing an argument for which there is no parameter"? There is no meaningful notion of "the type that results when passing an argument for which there is no parameter" in isolation.

Perhaps this means "parmN shall be of a non-reference type such that applying the default argument promotions to a prvalue of the parameter's type would not change its type"? But... this constraint seems pointless. What are we trying to defend against here? Why should this not be required to work:

void x(float f, ...) { // or with nullptr_t instead of float
va_list l; va_start(l, f);
// ...
}

? And what about the multitude of cases where passing an argument of that type with no corresponding parameter would be ill-formed (by being conditionally-supported), such as:

void y(std::string s, ...) {
va_list l; va_start(l, s);
// ...
}

?

@tkoeppe
Copy link
Contributor

tkoeppe commented Jun 8, 2016

@zygoloid: Yes, all good points. Indeed, the meaning is that the type of paramN must default-promote to itself. Why the rule is there only God knows, but indeed it seems that C does not want to allow void f(float, ...).

I'm not actually sure if we ever say whether void y(std::string s, ...) is allowed. Or a reference parameter. I agree that there doesn't appear to be a reason for this constraint.

@AaronBallman
Copy link
Contributor Author

What are we trying to defend against here?

@zygoloid: my understanding from talking to David is that this restriction is in place for platforms that want to implement va_start in pure C. He quoted the rationale document:

"The parmN argument to va_start was intended to be an aid to implementors writing the definition of a conforming va_start macro entirely in C, even using pre-C89 compilers (for example, by taking the address of a parameter). The restrictions on the declaration of the parmN parameter follow from the intent to allow this kind of implementation, as applying the & operator to a parameter name might not produce the intended result if the parameter's declaration did not meet these restrictions."

(Quote pulled from http://www.open-std.org/jtc1/sc22/wg14/www/C99RationaleV5.10.pdf)

@tkoeppe tkoeppe added the cwg Issue must be reviewed by CWG. label Nov 12, 2016
@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 12, 2016

I opened a Core issue for this, whose resolution will subsume this proposed change. Closing for now, and thank you for bringing this up!

@tkoeppe tkoeppe closed this Nov 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cwg Issue must be reviewed by CWG.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants