fitzgen requested cfallin for a review on PR #4701.
fitzgen opened PR #4701 from no-more-abi-callee-trait
to main
:
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
jameysharp submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
Constraints on
B
already give you that it's aLowerBackend
, so do you still have to be explicit about that? This is one part of Rust syntax I haven't really got down yet.abi: AbiCallee<<B::MInst as MachInst>::ABIMachineSpec>,
jameysharp created PR review comment:
:thinking: I guess simplifying this away is work for a later PR, huh?
jameysharp created PR review comment:
With the removal of the
flags
method, I wondered if theflags
field could be removed too. Looks like it's still used though.
jameysharp created PR review comment:
I guess this isn't "above" any more.
jameysharp created PR review comment:
I see this typo was in the original comment but I'll point it out anyway.
/// at a given program point (prior to emission of the safepointing
cfallin submitted PR review.
cfallin created PR review comment:
Can we keep the capitalization
ABICallee
? I know there are varying opinions on how camelcase interacts with acronyms; but I findAbi
for the acronymABI
a little unnatural to read. (Sorry for the bikeshed color suggestion!)
cfallin submitted PR review.
cfallin submitted PR review.
fitzgen created PR review comment:
I changed it because it is Rust's naming style to only capitalize the first letter of an acronym in a type (consistent with how you wouldn't add underscores between each letter in the acronym in snake case):
In <code class="hljs">UpperCamelCase</code>, acronyms and contractions of compound words count as one word: use <code class="hljs">Uuid</code> rather than <code class="hljs">UUID</code>, <code class="hljs">Usize</code> rather than <code class="hljs">USize</code> or <code class="hljs">Stdin</code> rather than <code class="hljs">StdIn</code>. In <code class="hljs">snake_case</code>, acronyms and contractions are lower-cased: <code class="hljs">is_xid_start</code>.
https://rust-lang.github.io/api-guidelines/naming.html
And also because I happen to find it more readable that way as well (no mixing up the "C" in "Callee" as part of the acronym). I plan on doing the rest in follow up PRs that I'm working on.
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
Yeah, don't want to balloon this PR.
fitzgen submitted PR review.
fitzgen created PR review comment:
Yeah I only removed things that were warned as dead code.
jameysharp submitted PR review.
jameysharp created PR review comment:
When there's a collision between widespread naming conventions and readability, I like to have... both? :sweat_smile:
Is there an alternative name that's meaningful but avoids the "ABI" acronym entirely? Or perhaps, would
CalleeAbi
work as a compromise? Somehow that reads less badly to me.
fitzgen created PR review comment:
The type checker was complaining it couldn't infer where the
MInst
projection was coming from, not sure why it can't be a little smarter here, but alas.
fitzgen submitted PR review.
fitzgen updated PR #4701 from no-more-abi-callee-trait
to main
.
cfallin created PR review comment:
I guess I diverge from the above guidelines then in this particular area :-) This paragraph from the "About" page of that book seems relevant too:
These are only guidelines, some more firm than others. In some cases they are vague and still in development. Rust crate authors should consider them as a set of important considerations in the development of idiomatic and interoperable Rust libraries, to use as they see fit. These guidelines should not in any way be considered a mandate that crate authors must follow, though they may find that crates that conform well to these guidelines integrate better with the existing crate ecosystem than those that do not.
I find
AbiCallee
looks wrong to me; an ABI is an ABI, not an Abi (or "abi"); I'd really prefer not to take the change. If it's confusing to me and not confusing to you, then there is likely a split among other readers as well. Given that it is currently this way and there is disagreement, could we keep with the current convention for this PR and then discuss separately? (I'd the same concern if the other names were also modified in this way.)
cfallin submitted PR review.
cfallin edited PR review comment.
cfallin submitted PR review.
cfallin created PR review comment:
I saw @jameysharp 's reply after writing mine -- but this brings to mind another option: just
Caller
andCallee
? (orCallerHandler
andCalleeHandler
?)
fitzgen submitted PR review.
fitzgen created PR review comment:
I find
ABI
grating as well though, so perhaps Jamey's suggestion is the best way to move forward here.Since it is already in an
abi
module, perhaps we can just call itCallee
(orabi::Callee
)?
jameysharp submitted PR review.
jameysharp created PR review comment:
Ooh, I like
abi::Callee
.
fitzgen updated PR #4701 from no-more-abi-callee-trait
to main
.
fitzgen updated PR #4701 from no-more-abi-callee-trait
to main
.
fitzgen has enabled auto merge for PR #4701.
fitzgen updated PR #4701 from no-more-abi-callee-trait
to main
.
bjorn3 created PR review comment:
Why this change?
bjorn3 submitted PR review.
fitzgen merged PR #4701.
fitzgen submitted PR review.
fitzgen created PR review comment:
Because it is public now that there is the
MachInst::ABIMachineSpec
associated type.
Last updated: Dec 23 2024 at 12:05 UTC