-
Notifications
You must be signed in to change notification settings - Fork 83
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
[reference] Added macro docs #70
Conversation
|
||
Using the `map` macro defined above | ||
|
||
```move |
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.
Will macros permit receiver syntax (now or ever)? If so, should we add an example of that here?
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 call!
I need to add a section, especially since it changes things a bit for evaluation order
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.
Overall looks very nice, just a couple small spelling/editing nits.
reference/src/functions/macros.md
Outdated
## Syntax | ||
|
||
`macro` functions have a similar syntax to normal functions. However, all type parameter names and | ||
all parameter names must start with a `$`. |
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.
"... with the exception of the _
type" possibly?
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.
Not sure I follow
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.
Can we have macro parameter named _
or it has to be $_
or none of these is allowed?
reference/src/functions/macros.md
Outdated
} | ||
``` | ||
|
||
The `$` is there to indicate that these do not behave like their normal, non-macro counterparts. For |
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.
What are "these"? Might be useful to specify what exactly they are.
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.
Overall LGTM just a couple small editing comments.
For example, the following `macro` function takes a vector and a lambda, and applies the lambda to | ||
each element of the vector to construct a new vector. |
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.
Consider moving this example to the top, so someone consulting the reference sees it first thing.
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.
Yeah getting an example early might be desirable. I hope that in most renderings this shows up on the page without having to scroll, which might be eye-catching enough?
Co-authored-by: Cam Swords <[email protected]> Co-authored-by: Tim Zakian <[email protected]>
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.
Small things, but otherwise LGTM
`vector::any` macro | ||
|
||
```move | ||
public macro fun any<$T>($v: &vector<$T>, $f: |&$T| -> bool): bool { |
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 may be a little confusing that the example for the "useful" case uses labelled return whereas the "original" one uses un-labelled return
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'm confused. I don't know what example you are comparing this one to?
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.
This one uses un-labelled return:
let result = apply!(|x| { if (x == 0) return 0; x + 1 }, 100);
And this one (which in text is mentioned as a more "useful" example of the one above uses labelled return:
v.do_ref!(|e| if ($f(e)) return 'any true);
It's minor, and it's probably OK to leave as is, but when I was reading this, I expected the first example and the second one to differ only in complexity rather than them using a subtly different control flow.
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 reworded to help this
reference/src/functions/macros.md
Outdated
|
||
Even though `foo()` will abort, it's return type can be used to start a method call. | ||
|
||
`$s` will not be evaluated if `$cond` is `false`. Son under a normal non-method call, an argument of |
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.
Can't suggest and edit for some reason using GitHub machinery so here it is as text:
`$s` will not be evaluated if `$cond` is `false`, and under a normal non-method call, an argument of
maybe_s!(foo(), false) // does not abort | ||
``` | ||
|
||
It becomes more clear as to why it does not abort when looking at the expanded form |
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.
Why don't we move the expanded example to before line 526? Then we can say something like "Looking at the expanded form, it is clear that foo() only get executed if the condition is false, so under the following non-method call, foo() is not called and execution will not abort"
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 added some additional wording which I think will help
reference/src/functions.md
Outdated
|
||
fun example() { | ||
let mut sum = 0; | ||
ntimes!(10, |i| sum = sum + i ); |
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.
Do we want to use a different variable name in lambda definition to avoid implying that it has to be somehow the same as what's used in the macro body?
reference/src/functions/macros.md
Outdated
## Syntax | ||
|
||
`macro` functions have a similar syntax to normal functions. However, all type parameter names and | ||
all parameter names must start with a `$`. |
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.
Can we have macro parameter named _
or it has to be $_
or none of these is allowed?
`vector::any` macro | ||
|
||
```move | ||
public macro fun any<$T>($v: &vector<$T>, $f: |&$T| -> bool): bool { |
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.
This one uses un-labelled return:
let result = apply!(|x| { if (x == 0) return 0; x + 1 }, 100);
And this one (which in text is mentioned as a more "useful" example of the one above uses labelled return:
v.do_ref!(|e| if ($f(e)) return 'any true);
It's minor, and it's probably OK to leave as is, but when I was reading this, I expected the first example and the second one to differ only in complexity rather than them using a subtly different control flow.
reference/src/functions/macros.md
Outdated
} | ||
``` | ||
|
||
The reason is that if the argument if `$x` was not a reference, it would be borrowed first, which |
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.
The reason is that if the argument if `$x` was not a reference, it would be borrowed first, which | |
The reason is that if the argument`$x` was not a reference, it would be borrowed first, which |
No description provided.