Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Macros to use path instead of ident #1474
Macros to use path instead of ident #1474
Changes from 27 commits
7102f28
933fd3f
dc43fa5
16668ca
ebd13e3
270a6b7
8be65aa
850a77e
5ae6e07
04dc503
95444ae
6a773d0
bc17557
6d9887b
35747a1
330af4a
10bf65c
97bfec4
6a07bb3
f303276
963be94
76de90d
32f2f1e
877fdc3
c89c0c7
8184085
1ee920b
5050625
55daf79
43ca2c2
2a2b45f
b55615b
d253b66
d2009d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
all of this seem to have been hardcoded 🙈 well, not like anything broke because of it, but indeed not a good practice.
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.
unless if was intentional? but I can't see a reason.
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 can't see a reason either, and we need this unhardcoded moving forward to work with the frame umbrella crate
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 recall this was one of the BIG BIG fixes in this PR when I was working on it.
Try this for yourself: if you are familiar with the tt_call pattern a bit, instead of passing in the
your_tt_return
, passframe_support
as it was in the past to the caller. Then have the caller call#frame_support::__private::tt_return
instead of plainyour_tt_return
. These two seem equivalent, but the former never worked.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.
But, hearing what I just said, in the scope of this PR, it is pretty unclear why we are doing this. I recall my working example was
minimal-runtime
. Would be good if you also bring back some sort of example here to demonstrate that this change is actually needed. Ideally, this example will just a test in frame-support and not theframe
crate.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.
Yes, that's what looks at a glance... did you find out what's the reason?
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.
@kianenigma Probably the best example is by using the frame-crate itself, which makes me reconsider if bringing this change here was a good idea and instead bring it in the 'main' frame umbrella PR. WDYT?
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 highly suspect that it didn't work before because the "receiver" didn't expect to see any "arguments" named
your_tt_return
and have been coded to only recognizeframe_support
as an "argument". If you take a look attt_default_parts.rs
, you'll see what I mean.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.
If my memory is good, the former didn't work because there was another error when giving a path instead of a single ident:
Like code was something like this:
but it should have been like this for path to work
but when you implemented with
your_tt_return
you fixed this kind of error.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.
macro_magic
might make this easier to reason about perhaps 😇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 specific workaround for passing paths to these btw is to do an interpolation of idents segmented by
::
and with an optional leading::
. For some reason that just works positionally wherepath
doesn't,see: https://github.com/sam0x17/macro_magic/blob/main/core/src/lib.rs#L511