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

Syntax of the zeroslice macro #3478

Closed
sffc opened this issue Jun 2, 2023 · 13 comments · Fixed by #3611
Closed

Syntax of the zeroslice macro #3478

sffc opened this issue Jun 2, 2023 · 13 comments · Fixed by #3611
Assignees
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake S-small Size: One afternoon (small bug fix or enhancement) T-core Type: Required functionality

Comments

@sffc
Copy link
Member

sffc commented Jun 2, 2023

Right now it is zeroslice![i16; somefunction; a, b, c, d]

I think maybe it should be:

  1. zeroslice!(i16; somefunction; [a, b, c, d])
  2. zeroslice!(i16, somefunction, [a, b, c, d])
  3. zeroslice!(type: i16, cnv: somefunction, data: [a, b, c, d])
  4. ... ?

Discuss with:

Optional:

@sffc sffc added discuss Discuss at a future ICU4X-SC meeting C-zerovec Component: Yoke, ZeroVec, DataBake labels Jun 2, 2023
@robertbastian
Copy link
Member

#1935

@robertbastian
Copy link
Member

I think if we can get rid of the aligned type we should not use named parameters, as that's not very rusty.

zeroslice![somefunction; a, b, c, d]

@sffc
Copy link
Member Author

sffc commented Jun 7, 2023

I think I prefer the [] to surround only the list: zeroslice!(somefunction, [a, b, c, d])

@robertbastian
Copy link
Member

The extra level of nesting might make formatting weird. Also the vec![] macro conventionally uses square brackets, which I'd like to mirror, and then zerovec![somefunction; [a, b, c, d]] is a bit brackety.

@sffc sffc added the discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band label Jun 15, 2023
@sffc
Copy link
Member Author

sffc commented Jun 15, 2023

  • @robertbastian - Don't like named arguments. If we use the conversion function with element syntax, the conversion function is always needed; it's not optional
  • @skius - There's a way to make an explicit conversion function without the types
  • @sffc - I see the conversion function as something we should always make available; we can start eliding it with const trait
  • @echeran - It seems like we want the macro to be extensible; so what's the argument against named arguments? We may take different sets of arguments, which means that we may get into positional arguments. Or if we want to add a fourth optional parameter, call sites are not poorly affected.
  • @robertbastian - Const traits are years away; we shouldn't let that constrain our planning here. We can make a breaking change to this in zerovec 2. In general, named parameters are not seen in Rust. We have one named parameter that is currently required and will be required for the forseable future. I don't see other changes coming into zerovec that could require named parameters.
  • @sffc - Const traits are already on nightly.
  • @sffc - Named arguments are available in attributes already, with = syntax.
  • @sffc - VarZeroVec could have different needs with different requirements for the arguments.
  • @robertbastian - Derive attributes are very different; I don't buy the parallelism with named arguments.
  • @robertbastian - If you omit the conversion function, it's fine.
  • @robertbastian - Const traits have been on nightly for years.
  • @echeran - I feel like in general, people are trying to move away from positional arguments. That's something Zibi has been saying from the get-go in MessageFormat. I see that in a lot of programming languages.
  • @skius - I think most of the syntaxes we've discussed should continue to work if we can elide the conversion function. So I think future compatibility shouldn't be an argument.
  • @robertbastian - I see this as different. We shouldn't make the invocation more verbose.
  • @sffc - We might need the type for using different behavior for primitives.
  • @echeran - Is this a drop-in replacement for vec! ? If there are edge cases... I think it's good to make these readable.
  • @skius - See Magic zeroslice! macros #3454 (comment) for details on the edge cases where we may want the type parameter.
  • @robertbastian - The 5 extra characters are characters you'll always need to type out and there's no auto-completion.
  • @sffc - Let's talk about ( vs [. When we get to the point about eliding the conversion function, then we can use [. But when we have the conversion function argument, then I prefer ([])
  • @skius - If there is only one level of brackets, square brackets is better, but if they are nested, then it seems good for them to be separate.
  • @echeran - If it's not exactly consistent with vec!, then it seems like it's better to be more like other function macros with ().
  • @robertbastian - I think it should be a drop-in replacement for vec! and use vec!-like syntax. The double nesting could give us weird formatting as well. Also, double nesting loses screen real estate.
  • @sffc - This macro is very different than vec!: it can't be assigned to non-const variables, and it requires a conversion function.
  • @sffc - Also, there's no precedent for ; followed by a comma-separated list.
  • @skius - Another possibility: zerovec!([a1, a2, ...].map(<conversion fn>)) - quick suggestion, probably too confusing to use
  • @robertbastian - zerovec![<conversion fn>(a,b,c,d)]

No conclusion at this time. Would like feedback from others such as @Manishearth or @zbraniecki

@Manishearth
Copy link
Member

No strong position, feel like zeroslice![ty; conversion; a, b, c, d] could work

@sffc
Copy link
Member Author

sffc commented Jun 16, 2023

What makes that syntax desirable? It violates every point I raised in the conversation:

  • Semicolon followed by comma-separated list has no precedent and is just weird
  • Lack of named arguments means we can't safely elide the conversion function or the type (unless we choose the model where there is never a type, but I'm not ready to make that call at this time)
  • Lack of named arguments makes it less readable
  • It is not vec! and can never be a drop-in replacement; it is a function that transforms things from one state to another

@sffc
Copy link
Member Author

sffc commented Jun 16, 2023

The strongest argument I heard in favor of a flat syntax (no nesting) is less indentation when formating. But, does the formatter even know what to do with the semicolon-followed-by-list?

@Manishearth
Copy link
Member

I think it's not that weird because [val; N] syntax uses semicolons to separate heterogenous things.

I think you can still elide things: one semicolon means ty; values, two means ty; conversion; values. The zero-value case is the only one where there's potential ambiguity and you don't actually need a conversion function for that.

I think it can be a drop in replacement by adding a zero-semicolon case eventually.

Anyway, it was just a suggestion, not something I am strongly in favor of.

@sffc
Copy link
Member Author

sffc commented Jun 16, 2023

I think it's not that weird because [val; N] syntax uses semicolons to separate heterogenous things.

I agree for two single items, but it's unprecedented to be followed by a comma-separated list; in the best case, I need to squint to see where the semicolons end and the commas begin.

I think you can still elide things: one semicolon means ty; values, two means ty; conversion; values.

I was trying to make the case that in general we don't know what to elide:

  1. We need $ty when assigning to a non-const variable and potentially when special casing primitives
  2. We need $cnv until const traits are ready, and we could decide to elide it ahead of time with a duck-type function
  3. VarZeroVec may need yet another named parameter

Those cases are different sets. There may be cases where you need both $ty and $cnv, only $ty, only $cnv, or neither.

The zero-value case is the only one where there's potential ambiguity and you don't actually need a conversion function for that.

Good point about zero-value cases. It should be more explicit when you are having zero values. I don't want to read, parse, or generate syntax that requires special zero-value handling.

I think it can be a drop in replacement by adding a zero-semicolon case eventually.

Yes, but any of the other proposals can also have that end goal.

@robertbastian
Copy link
Member

We need $ty when assigning to a non-const variable and potentially when special casing primitives

The non-const variable case can be solved with inline const, which iiuc is a lot closer than const traits: rust-lang/rust#76001. I think it's fine to not do $ty, and let clients constify until we get inline const.

VarZeroVec may need yet another named parameter

VZV and ZV can take different parameters without doing named parameters.

The zero-value case is the only one where there's potential ambiguity and you don't actually need a conversion function for that.

The zero-value case is already special cased to ZeroVec::new, where there's no need to involve types, ULE, or intermediate consts.

@sffc
Copy link
Member Author

sffc commented Jun 19, 2023

One more point I should raise: although I suggested the approach of calling the conversion function for each element in order to avoid the type parameter, I'm not actually convinced that it's the technologically superior solution than using the function to convert the whole array in one go. The generated code is thousands of function calls instead of one function call with a loop.

@sffc
Copy link
Member Author

sffc commented Jul 3, 2023

  • @skius - In areas where the type name or conversion function are rather long, I can see an argument that a clearer separation between arguments and values could be usable. Which could speak for the nested brackets over a simple semicolon.
  • @zbraniecki - In my experience macros are using positional arguments. So I would lean toward 1 or 2 over 3 (from the list in the OP).
  • @sffc - We never really aligned my hypothesis that we want both of these arguments to be independently optional.
  • @Manishearth - We can change things later; it is zerovec. The point of this is to be convenient. So I like syntax 1. Not a huge fan of the nested parentheses but it's clearer.
  • @sffc - I'm okay with option 1 if we can say that we'll revisit how to do optional arguments when we are able to do so down the road.

Consensus:

  • Syntax: zeroslice!(i16; somefunction; [a, b, c, d])
  • No consensus on the syntax when either of the first two parameters is omitted

LGTM: @sffc @skius @robertbastian @zbraniecki @Manishearth

@sffc sffc removed discuss Discuss at a future ICU4X-SC meeting discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band labels Jul 3, 2023
@sffc sffc added this to the 1.x Priority ⟨P2⟩ milestone Jul 3, 2023
@sffc sffc added S-small Size: One afternoon (small bug fix or enhancement) T-core Type: Required functionality labels Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake S-small Size: One afternoon (small bug fix or enhancement) T-core Type: Required functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants