forked from rust-lang/rfcs
-
Notifications
You must be signed in to change notification settings - Fork 0
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
some editing to better reflect my concern #1
Merged
oli-obk
merged 2 commits into
oli-obk:const_generic_const_fn_bounds
from
RalfJung:const_generic_const_fn_bounds
Jun 25, 2020
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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 did not edit this part, but given that the exact same thing is called "natural" for
dyn Trait
, I feel we should adjust one of the two sentences. :)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's not my opinion. Iirc I just added it because someone mentioned it.
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.
So what about removing both this and the "natural", basically avoiding making a judgment?
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 still think it's "natural" or at least consistent to keep
dyn some trait specifiers
in sync withT: some trait specifiers
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 agree with that. And moreover, I think it is just as natural to keep
dyn <specifiers> Trait
in sync with<specifiers> fn
(both of these are types here, not items/delcarations). Conversely, I think an asymmetry between our two kinds of dynamic dispatch (fn ptrs anddyn Trait
) is just as unnatural as an asymmetry between traits dispatching statically or dynamically.So I am fine with keeping the "natural" here for traits if we also call the same thing "natural" for function pointers.
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.
extern
fn ptrs make no sense --extern
is a property of a function definition, but irrelevant for its type. (We haveextern "ABI" fn
ptrs, but hereextern
is just a bad keyword that we use to explicitly specify the ABI.extern "Rust" fn
is the exact same thing asfn
.)We don't need
dyn unsafe Trait
because theTrait
declares its methods and whether they areunsafe
. That is just like saying that function pointers come with argument and return types whichdyn Trait
does not -- it is true, and IMO does not change the argument here. It just reflects thatdyn Trait
is nominal while function pointers are structural.Imagine a structural version of
dyn Trait
. It would bedyn trait { fn method1(i32); unsafe fn method2(i64); }
. Now afn
ptr is very much the same thing as adyn trait {...}
with a single function. Does this make the symmetry clearer?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.
yea, ok, the symmetry in semantics is clear, but I was mainly worried about syntax 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.
Ah -- I don't care about syntax, as long as it is consistent. ;) Which is what this is not, hence my comments.
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 let's do that.
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.
Done. I also reordered the fn ptr and dyn Trait discussion, because as you said dyn Trait is closer to what the RFC discusses otherwise, so this makes for a smoother transition I think.