-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Proposal: Variadics #2240
base: trunk
Are you sure you want to change the base?
Proposal: Variadics #2240
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made another pass without diving into the type checking bits, since those seem like something that could be handled in a separate proposal once the other issues are handled.
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I really appreciate the examples you have included, they are quite helpful (and so I've asked for even more!).
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I think this is ready for leads review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all but the appendix (largely trusting Josh's review there) and generally pretty happy across the board. Left a bunch of comments, but mostly minor wording / presentation improvements.
proposals/p2240.md
Outdated
template <typename T> | ||
requires std::totally_ordered<T> && std::copyable<T> | ||
T Min(const T& t) { | ||
return t; | ||
} | ||
|
||
template <typename T, typename... Params> | ||
requires std::totally_ordered<T> && std::copyable<T> && | ||
(std::same_as<T, Params> && ...) | ||
T Min(T first, Params... rest) { | ||
T min_rest = Min(rest...); | ||
if (min_rest < first) { | ||
return min_rest; | ||
} else { | ||
return first; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I think it might be worth rewriting the C++ examples to use if constexpr
to provide the base case without overloading as that seems a bit briefer and more representative, here and below. WDYT?
Here is what I think this one would look like:
template <typename T> | |
requires std::totally_ordered<T> && std::copyable<T> | |
T Min(const T& t) { | |
return t; | |
} | |
template <typename T, typename... Params> | |
requires std::totally_ordered<T> && std::copyable<T> && | |
(std::same_as<T, Params> && ...) | |
T Min(T first, Params... rest) { | |
T min_rest = Min(rest...); | |
if (min_rest < first) { | |
return min_rest; | |
} else { | |
return first; | |
} | |
} | |
template <typename T, typename... Params> | |
requires std::totally_ordered<T> && std::copyable<T> && | |
(std::same_as<T, Params> && ...) | |
T Min(T first, Params... rest) { | |
// Base case. | |
if constexpr (sizeof...(rest) == 0) { | |
return first; | |
} else { | |
T min_rest = Min(rest...); | |
if (min_rest < first) { | |
return min_rest; | |
} else { | |
return first; | |
} | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put the // Base case.
comment inside the first branch of the if
.
I feel very mixed about this suggestion since the overloading approach was the way to do this in the day, but I guess C++ has moved and the example is documented as using C++20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell this technique only works if the minimum arity is at least 1, so that the signature can do the destructuring while still supporting all usages. That makes it a great fit for Min
, but awkward to apply to StrCat
. I've done it (see below), but I had to rely on the fact that there's a separate implementation function (which is basically a coincidence), and the result still doesn't seem like a net improvement to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this if constexpr
trick is an improvement for StrCat
, where it creates two edge cases instead of one. @zygoloid had an alternate implementation that uses fold expressions to avoid quadratic compile time and the need for a helper function, but I don't know if that serves the narrative purpose you are looking for. Is this supposed to be a common way of writing this function, a way that shows common pitfalls, or a way that is as similar as possible to the Carbon approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had been aiming for these examples to be representative of how functions like this would typically be written. I think that means we shouldn't use the comma-fold trick, and probably not if constexpr
either, although that's more debatable. However, it sounds like maybe Chandler is asking for the examples to represent the ideal versions of these functions; in that case, it might make sense to use the comma-fold trick in StrCat
. In either case, I agree with you that StrCat
should not use if constexpr
, but I'll wait for @chandlerc to clarify what he'd like to see.
proposals/p2240.md
Outdated
This approach has some major advantages: the keywords are more consistent with | ||
`each` (and `expand` to some extent), substantially less visually noisy than | ||
`...`, and they may also be more self-explanatory. However, it does have some | ||
substantial drawbacks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we should mention & credit the Swift syntax explicitly here, which is similar but not exactly the same as this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. How's this?
|
||
<!-- tocstop --> | ||
|
||
## Basics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be useful to have a few sub-sections of this section to help the reader keep track of where things are... At the least, the point where this switches from pack expansion expressions / statements into pack patterns is a bit easy to miss I find.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, how's this?
A pack literal can be _expanded_, which moves its parent AST node inside the | ||
pack literal, so long as the parent node is not `...`. For example, | ||
`... Optional(⟬each X, Y⟭)` is equivalent to | ||
`... ⟬Optional(each X), Optional(Y)⟭`. Similarly, an arity coercion can be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find "parent AST node" a strange concept to introduce here. This is a language design, and not really a compiler design, so it seems a bit circular to talk about AST nodes.
Maybe "moves its parent delimited syntactic construct inside the pack literal"? "deliminited syntactic construct" seems to be a reasonable description to cover the cases you care about here maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I would read "delimited syntactic construct" to mean that there need to be delimiters, such as the parentheses in this example, but that's not the intent; the example could just as well be that ... 2 * ⟬each X, Y⟭
is equivalent to ... ⟬2 * each X, 2 * Y⟭
. "Syntactic construct" might work, but it seems awfully vague.
Would "syntax tree node" work better for you?
It's possible that any way of expressing this is going to seem a little strange, because the language rule I'm describing here is a little strange. Even the "formal" version of this rule in the appendix (starting around line 704) winds up being pretty handwavy, because it can't be expressed as a reduction rule; it's really a rule for generating reduction rules.
proposals/p2240.md
Outdated
template <typename T> | ||
requires std::totally_ordered<T> && std::copyable<T> | ||
T Min(const T& t) { | ||
return t; | ||
} | ||
|
||
template <typename T, typename... Params> | ||
requires std::totally_ordered<T> && std::copyable<T> && | ||
(std::same_as<T, Params> && ...) | ||
T Min(T first, Params... rest) { | ||
T min_rest = Min(rest...); | ||
if (min_rest < first) { | ||
return min_rest; | ||
} else { | ||
return first; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put the // Base case.
comment inside the first branch of the if
.
I feel very mixed about this suggestion since the overloading approach was the way to do this in the day, but I guess C++ has moved and the example is documented as using C++20.
Co-authored-by: Chandler Carruth <chandlerc@gmail.com> Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
proposals/p2240.md
Outdated
```cpp | ||
template <ConvertibleToString... Ts> | ||
std::string StrCat(const Ts&... params) { | ||
std::string result; | ||
result.reserve((params.Length() + ... + 0)); | ||
StrCatImpl(&result, params...); | ||
return result; | ||
if constexpr (sizeof...(params) == 0) { | ||
return ""; | ||
} else { | ||
std::string result; | ||
result.reserve((params.Length() + ... + 0)); | ||
StrCatImpl(&result, params...); | ||
return result; | ||
} | ||
} | ||
|
||
void StrCatImpl(std::string* out) { return; } | ||
|
||
template <ConvertibleToString T, ConvertibleToString... Ts> | ||
void StrCatImpl(std::string* out, const T& first, const Ts&... rest) { | ||
out->append(first.ToString()); | ||
StrCatImpl(out, rest...); | ||
if constexpr (sizeof...(rest) > 0) { | ||
StrCatImpl(out, rest...); | ||
} | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josh11b asked me whether I'd write / prefer the form with if constexpr
(with two termination conditions) or the form with the extra function overload. I said I'd actually write this:
```cpp | |
template <ConvertibleToString... Ts> | |
std::string StrCat(const Ts&... params) { | |
std::string result; | |
result.reserve((params.Length() + ... + 0)); | |
StrCatImpl(&result, params...); | |
return result; | |
if constexpr (sizeof...(params) == 0) { | |
return ""; | |
} else { | |
std::string result; | |
result.reserve((params.Length() + ... + 0)); | |
StrCatImpl(&result, params...); | |
return result; | |
} | |
} | |
void StrCatImpl(std::string* out) { return; } | |
template <ConvertibleToString T, ConvertibleToString... Ts> | |
void StrCatImpl(std::string* out, const T& first, const Ts&... rest) { | |
out->append(first.ToString()); | |
StrCatImpl(out, rest...); | |
if constexpr (sizeof...(rest) > 0) { | |
StrCatImpl(out, rest...); | |
} | |
} | |
``` | |
```cpp | |
template <ConvertibleToString... Ts> | |
std::string StrCat(const Ts&... params) { | |
std::string result; | |
result.reserve((params.Length() + ... + 0)); | |
(out->append(params.ToString()), ...); | |
return result; | |
} | |
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me too, but I don't think that's representative of what a typical C++ programmer would do in this situation, so this gets back to the question of whether we want the C++ examples to reflect typical usage or expert usage (see also this comment thread).
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Proposes a set of core features for declaring and implementing generic variadic
functions.
A "pack expansion" is a syntactic unit beginning with
...
, which is a kind ofcompile-time loop over sequences called "packs". Packs are initialized and
referred to using "pack bindings", which are marked with the
each
keyword atthe point of declaration and the point of use.
The syntax and behavior of a pack expansion depends on its context, and in some
cases by a keyword following the
...
:...
iteratively evaluates its operand expression, and treats the values as
successive elements of the tuple.
...and
and...or
iteratively evaluate a boolean expression, combiningthe values using
and
andor
, and ending the loop early if the underlyingoperator short-circuits.
...
iteratively executes a statement....
iteratively matches the elements of the scrutinee tuple. In conjunction with
pack bindings, this enables functions to take an arbitrary number of
arguments.