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

Wrap the imf_version module #20

Merged
merged 10 commits into from
May 16, 2021

Conversation

scott-wilson
Copy link
Member

Description

Closes #18.

Implement the imfVersion module.

This changes the API from what's in C++ to make the version and flags have their own type, instead of being a simple integer.

CPPMM Checklist

Cppmm is our bindings generator. There are some tools in it to aid in creating safe and stable APIs.

  • Use CPPMM_RENAME(ctor) for the "main" type constructor.
  • Add CPPMM_THROWS for any code that may throw an exception in C++.

FFI Safety Checklist

The Rustonomicon provides useful guides to prevent soundness errors in Rust. The FFI section of the document provides the source for these checklist items.

  • Null pointers are properly handled
  • Panics in Rust never cross FFI boundary
    • There is code that can panic (openexr::version::Version::new(...)), but this shouldn't be called on the C++ side.
  • Types implement drop trait for C/C++ destructors
  • Concurrency is properly managed between Rust and C/C++
  • C style strings are converted to CStr or CString
  • Wrap non-trivially movable types in a Box

Rust API Guidelines Checklist

The checklist is heavily inspired by The Rust API Guidelines. Please check each item that applies, or note if an item is intentionally skipped (either partially or fully) with the reason.

  • Naming (crate aligns with Rust naming conventions)
    • Casing conforms to RFC 430 (C-CASE)
    • Ad-hoc conversions follow as_, to_, into_ conventions (C-CONV)
    • Getter names follow Rust convention (C-GETTER)
    • Methods on collections that produce iterators follow iter, iter_mut, into_iter (C-ITER)
    • Iterator type names match the methods that produce them (C-ITER-TY)
    • Feature names are free of placeholder words (C-FEATURE)
    • Names use a consistent word order (C-WORD-ORDER)
  • Interoperability (crate interacts nicely with other library functionality)
    • Types eagerly implement common traits (C-COMMON-TRAITS)
      • Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug,
        Display, Default
    • Conversions use the standard traits From, AsRef, AsMut (C-CONV-TRAITS)
    • Collections implement FromIterator and Extend (C-COLLECT)
    • Data structures implement Serde's Serialize, Deserialize (C-SERDE)
    • Types are Send and Sync where possible (C-SEND-SYNC)
    • Error types are meaningful and well-behaved (C-GOOD-ERR)
    • Binary number types provide Hex, Octal, Binary formatting (C-NUM-FMT)
    • Generic reader/writer functions take R: Read and W: Write by value (C-RW-VALUE)
  • Macros (crate presents well-behaved macros)
  • Documentation (crate is abundantly documented)
  • Predictability (crate enables legible code that acts how it looks)
    • Smart pointers do not add inherent methods (C-SMART-PTR)
    • Conversions live on the most specific type involved (C-CONV-SPECIFIC)
    • Functions with a clear receiver are methods (C-METHOD)
    • Functions do not take out-parameters (C-NO-OUT)
    • Operator overloads are unsurprising (C-OVERLOAD)
    • Only smart pointers implement Deref and DerefMut (C-DEREF)
    • Constructors are static, inherent methods (C-CTOR)
  • Flexibility (crate supports diverse real-world use cases)
    • Functions expose intermediate results to avoid duplicate work (C-INTERMEDIATE)
    • Caller decides where to copy and place data (C-CALLER-CONTROL)
    • Functions minimize assumptions about parameters by using generics (C-GENERIC)
    • Traits are object-safe if they may be useful as a trait object (C-OBJECT)
  • Type safety (crate leverages the type system effectively)
    • Newtypes provide static distinctions (C-NEWTYPE)
    • Arguments convey meaning through types, not bool or Option (C-CUSTOM-TYPE)
    • Types for a set of flags are bitflags, not enums (C-BITFLAG)
    • Builders enable construction of complex values (C-BUILDER)
  • Dependability (crate is unlikely to do the wrong thing)
  • Debuggability (crate is conducive to easy debugging)
  • Future proofing (crate is free to improve without breaking users' code)
    • Sealed traits protect against downstream implementations (C-SEALED)
    • Structs have private fields (C-STRUCT-PRIVATE)
    • Newtypes encapsulate implementation details (C-NEWTYPE-HIDE)
    • Data structures do not duplicate derived trait bounds (C-STRUCT-BOUNDS)
  • Necessities (to whom they matter, they really matter)
    • Public dependencies of a stable crate are stable (C-STABLE)
    • Crate and its dependencies have a permissive license (C-PERMISSIVE)

Testing Checklist

The tests are meant to validate that the wrappers are sound and work as expected. Tests that are designed to exercise code for the wrapped library and not the bindings should be added to the wrapped library instead.

  • unsafe {} code is properly sanitized
  • All functions are called at least once in tests
  • Functions that can round trip data (for example, readers and writers, getters and setters, etc) are verified so that the input and output data are the same (if applicable).

Copy link
Contributor

@anderslanglands anderslanglands left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wunderbar lgtm

@anderslanglands anderslanglands merged commit 12dbf3e into vfx-rs:main May 16, 2021
@scott-wilson scott-wilson deleted the 18-bind_imf_version branch May 16, 2021 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bind bind/imf_version.cpp
2 participants