Improve the integer discriminant logic

It now bases integer discriminators on the prior variant when not
otherwise specified.

This also makes string discriminators the default.

CHANGELOG.md

@@ -1,7 +1,11 @@
 # 0.5
 
+  * All enums now default to `#[protocol(discriminant = "string")]`
+      * This discriminator type allows the most flexibility in future protocol changes.
+      * The old behaviour can still be achieved via `#[protocol(discriminant = "integer")]`
   * Add `protocol::Settings` to Parcel API for byte order configuration
       * Every `Parcel` implementation must be adjusted to include the new argument
+  * The `#[repr(integer type)]` enum attribute is now recognized and respected.
 
 # 0.4 and below
 

README.md

@@ -115,18 +115,19 @@ fn main() {
 
 Enum values can be transmitted either by their 1-based variant index, or by transmitting the string name of each variant.
 
-**NOTE:** The default behaviour is to use *the 1-based variant index* (`integer`).
+**NOTE:** The default behaviour is to use *the variant name as a string* (`string`).
 
-This behaviour can be changed by the `#[protocol(discriminant = "string")]` attribute.
+This behaviour can be changed by the `#[protocol(discriminant = "<type>")]` attribute.
 
 Supported discriminant types:
 
-* `integer` (default)
-    * This transmits the 1-based variant number as the over-the-wire discriminant
-    * Enum variants cannot be reordered in the source without breaking the protocol
-* `string`
+* `string` (default)
     * This transmits the enum variant name as the over-the-wire discriminant
     * This uses more bytes per message, but it very flexible
+* `integer
+    * This transmits the 1-based variant number as the over-the-wire discriminant
+    * If enum variants have explicit discriminators, the
+    * Enum variants cannot be reordered in the source without breaking the protocol
 
 **N.B.** `string` should become the default discriminant type for `#[derive(Protocol)]`. This would be a breaking change.
 Perhaps a major release?

protocol-derive-tests/src/enums.rs

@@ -113,6 +113,19 @@ mod integer_discriminants {
         Second = 2,
     }
 
+    #[derive(Protocol, Debug, PartialEq, Eq)]
+    #[protocol(discriminant = "integer")]
+    #[repr(i8)]
+    enum WithoutExplicitDiscriminators {
+        Only,
+    }
+
+    #[test]
+    fn discriminator_zero_is_reserved() {
+        assert_eq!(vec![1],
+                   WithoutExplicitDiscriminators::Only.raw_bytes(&protocol::Settings::default()).unwrap());
+    }
+
     #[test]
     fn named_fields_are_correctly_written() {
         assert_eq!(vec![0, 0, 0, 1, 1], BoatKind::Speedboat {

protocol-derive/src/format.rs

@@ -5,6 +5,8 @@ use syn;
 
 pub const DEFAULT_INT_DISCRIMINATOR_TYPE: &'static str = "u32";
 
+pub const DEFAULT_ENUM_DISCRIMINATOR_FORMAT: Enum = Enum::StringDiscriminator;
+
 /// Represents a format.
 pub trait Format : Clone {
     /// From a string.
@@ -24,14 +26,37 @@ impl Enum {
     /// Gets the discriminator of an enum variant.
     pub fn discriminator(&self, e: &syn::DataEnum,
                          variant: &syn::Variant) -> ::proc_macro2::TokenStream {
+        let allow_explicit_discriminators = match *self {
+            Enum::IntegerDiscriminator => true,
+            _ => false,
+        };
+
+        if !allow_explicit_discriminators {
+            for variant in e.variants.iter() {
+                if let Some(_) = variant.discriminant {
+                    panic!("only enums with integer discriminants may use explicit discriminants: '{}",
+                           quote!(#variant).to_string());
+                }
+            }
+        }
+
         match *self {
             Enum::IntegerDiscriminator => {
                 let variant_index = e.variants.iter().position(|v| v.ident == variant.ident).expect("variant not a part of enum");
+                let prior_variants: Vec<_> = e.variants.iter().collect();
+                let previous_discriminator = prior_variants.into_iter().rev().filter_map(|variant| {
+                    match variant.discriminant {
+                        Some((_, syn::Expr::Lit(syn::ExprLit { lit: syn::Lit::Int(ref n), .. }))) => Some(n.value()),
+                        _ => None,
+                    }
+                }).next()
+                  .unwrap_or_else(|| variant_index as u64 + 1); // incase no explicit discriminators
+                let default_discriminator = previous_discriminator + 1;
 
                 let discriminator = match variant.discriminant {
                     Some((_, syn::Expr::Lit(syn::ExprLit { lit: syn::Lit::Int(ref n), .. }))) => n.clone(),
                     Some(_) => panic!("unknown discriminator"),
-                    // Reserve discriminator 0.
+                    // If no explicit discriminant exists
                     None => syn::LitInt::new(variant_index as u64 + 1, syn::IntSuffix::None,
                                              proc_macro2::Span::call_site()),
                 };

protocol-derive/src/lib.rs

@@ -178,7 +178,8 @@ fn impl_parcel_for_enum(ast: &syn::DeriveInput,
     let enum_name = &ast.ident;
     let anon_const_name = syn::Ident::new(&format!("__IMPL_PARCEL_FOR_{}", ast.ident), proc_macro2::Span::call_site());
 
-    let format = attr::discriminant_format::<format::Enum>(&ast.attrs).unwrap_or(format::Enum::IntegerDiscriminator);
+    let format = attr::discriminant_format::<format::Enum>(&ast.attrs)
+                     .unwrap_or(format::DEFAULT_ENUM_DISCRIMINATOR_FORMAT);
 
     let discriminator_ty = match format {
         format::Enum::IntegerDiscriminator => {