Stream: git-wasmtime

Topic: wasmtime / PR #4701 Cranelift: Remove `ABICallee` trait

Wasmtime GitHub notifications bot (Aug 12 2022 at 00:02):

fitzgen requested cfallin for a review on PR #4701.

Wasmtime GitHub notifications bot (Aug 12 2022 at 00:02):

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.
-->

Wasmtime GitHub notifications bot (Aug 12 2022 at 01:00):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 12 2022 at 01:00):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 12 2022 at 01:00):

jameysharp created PR review comment:

Constraints on B already give you that it's a LowerBackend, 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>,

Wasmtime GitHub notifications bot (Aug 12 2022 at 01:00):

jameysharp created PR review comment:

:thinking: I guess simplifying this away is work for a later PR, huh?

Wasmtime GitHub notifications bot (Aug 12 2022 at 01:00):

jameysharp created PR review comment:

With the removal of the flags method, I wondered if the flags field could be removed too. Looks like it's still used though.

Wasmtime GitHub notifications bot (Aug 12 2022 at 01:00):

jameysharp created PR review comment:

I guess this isn't "above" any more.

Wasmtime GitHub notifications bot (Aug 12 2022 at 01:00):

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

Wasmtime GitHub notifications bot (Aug 12 2022 at 06:32):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 12 2022 at 06:32):

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 find Abi for the acronym ABI a little unnatural to read. (Sorry for the bikeshed color suggestion!)

Wasmtime GitHub notifications bot (Aug 12 2022 at 06:32):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 12 2022 at 06:35):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 15:59):

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.

Wasmtime GitHub notifications bot (Aug 15 2022 at 15:59):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:04):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:04):

fitzgen created PR review comment:

Yeah, don't want to balloon this PR.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:05):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:05):

fitzgen created PR review comment:

Yeah I only removed things that were warned as dead code.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:05):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:05):

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.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:06):

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.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:06):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:07):

fitzgen updated PR #4701 from no-more-abi-callee-trait to main.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:12):

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.)

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:12):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:12):

cfallin edited PR review comment.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:16):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:16):

cfallin created PR review comment:

I saw @jameysharp 's reply after writing mine -- but this brings to mind another option: just Caller and Callee? (or CallerHandler and CalleeHandler?)

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:17):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 16:17):

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 it Callee (or abi::Callee)?

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:03):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:03):

jameysharp created PR review comment:

Ooh, I like abi::Callee.

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:08):

fitzgen updated PR #4701 from no-more-abi-callee-trait to main.

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:12):

fitzgen updated PR #4701 from no-more-abi-callee-trait to main.

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:13):

fitzgen has enabled auto merge for PR #4701.

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:17):

fitzgen updated PR #4701 from no-more-abi-callee-trait to main.

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:42):

bjorn3 created PR review comment:

Why this change?

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:42):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 18:27):

fitzgen merged PR #4701.

Wasmtime GitHub notifications bot (Aug 15 2022 at 19:56):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 19:56):

fitzgen created PR review comment:

Because it is public now that there is the MachInst::ABIMachineSpec associated type.

Last updated: Nov 22 2024 at 17:03 UTC