Skip to content
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

Naming conventions #190

Open
bsekura opened this issue Dec 4, 2020 · 5 comments
Open

Naming conventions #190

bsekura opened this issue Dec 4, 2020 · 5 comments

Comments

@bsekura
Copy link
Contributor

bsekura commented Dec 4, 2020

TLDR
Take Metal Objective C and Swift API and change names to Rust idiomatic names where applicable.

General

Rust bindings are based on Objective C protocol bindings.
Follow Objective C protocol definition to determine properties and methods.

Swift API adds some higher level stuff that is beyond the scope of Rust bindings.

Struct (Object) and Enum names

Same as Objective C/Swift API, e.g.

MTLDevice, MTLCommandBufferStatus, MTLCommandBuffer etc.

Enums

Swift

enum MTLCommandBufferStatus : UInt {
  case notEnqueued
  case enqueued
  case committed
  case scheduled
  case completed
  case error
}

Rust: change variant names following Rust conventions:
(basically capitalize first word)

pub enum MTLCommandBufferStatus {
  NotEnqueued,
  Enqueued,
  Committed,
  Scheduled,
  Completed,
  Error,
}

NOTE: Some enums contain Apple brand names that intentionally start with lowercase.
In those cases respect that, adding necessary #allow to keep Rust happy. For example:

#[allow(non_camel_case_types)]
#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
enum OS {
    iOS,
    tvOS,
    macOS,
}

Method names

Objective C

- (void)addCompletedHandler:(MTLCommandBufferHandler)block;

Swift

func addCompletedHandler(_ block: @escaping MTLCommandBufferHandler)

Rust: change to lowercase snake case:

pub fn add_completed_handler(&self, block: &CommandBufferHandler)

Special Case

Some methods create new objects (aka factory methods).

Objective C:

- (id<MTLComputeCommandEncoder>)computeCommandEncoderWithDescriptor:(MTLComputePassDescriptor *)computePassDescriptor;
- (id<MTLComputeCommandEncoder>)computeCommandEncoderWithDispatchType:(MTLDispatchType)dispatchType;
- (id<MTLComputeCommandEncoder>)computeCommandEncoder;

Swift:
NOTE: overloading

func makeComputeCommandEncoder(descriptor computePassDescriptor: MTLComputePassDescriptor) -> MTLComputeCommandEncoder?
func makeComputeCommandEncoder(dispatchType: MTLDispatchType) -> MTLComputeCommandEncoder?
func makeComputeCommandEncoder() -> MTLComputeCommandEncoder?

Rust:

  • use Objective C name (we don't want overloading)
  • change to lowercase snake case as usual
  • follow Swift way to emphasize the intention; change make to new, as we do in Rust:
pub fn new_compute_command_encoder_with_descriptor(desc: MTLComputePassDescriptor) -> &ComputeCommandEncoderRef {}
pub fn new_compute_command_encoder_with_dispatch_type(&self, ty: MTLDispatchType) -> &ComputeCommandEncoderRef {}
pub fn new_compute_command_encoder(&self) -> &ComputeCommandEncoderRef {}
@chinedufn
Copy link
Collaborator

Awesome! Thanks so much for putting this together and including examples!

This looks fine to me.

If @kvark doesn't have any issues, want to just PR this into guide/naming-conventions/README.md.

Really like that you included examples, this was easy to follow.

Thanks!

@kvark
Copy link
Member

kvark commented Dec 14, 2020

My ❤️ emoji agrees with you!

@xixixao
Copy link
Contributor

xixixao commented Jan 12, 2021

What about MTLComputeCommandEncoder::new_with_descriptor(), would that be possible?

@kvark
Copy link
Member

kvark commented Jan 12, 2021

That would be an improvement, I think.

@kvark
Copy link
Member

kvark commented Jan 13, 2021

We want to release gfx-backend-metal very soon. Would anybody want to help with these naming changes in metal-rs? It's not a blocker for us, we can release an intermediate version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants