Generate different storage types macros based on features

[Nemo157]

Fixes #61

Adds a separate testcrate that attempts to build the examples as well to show that they work without any features enabled.

[iliekturtles]

Thanks for the PR! I've just skimmed so far and will review in more detail tomorrow. Another thought I had was to generate consts inside the uom crate and then wrap the contents of the match arms with if statements:

pub const U32_ENABLED: bool = cfg!(feature = "u32");

macro_rules! storage_types {
    (...) => {
        if($crate::storage_types::U32_ENABLED) {
            ...
        }
    },
}

[iliekturtles]

Thought about this some more and realized an if(CONST) won't work because an if can't be in an item position.

[iliekturtles]

The test crate doesn't seem to be setup correctly, or I'm misunderstanding the setup, as adding a compile error into any of the examples and then running cargo test --manifest-path testcrate/Cargo.toml --verbose succeeds.

I'd also like to see the test crate given a more descriptive name and moved out of the repository root. If moved into a sub directory of the examples folder do you know if cargo will automatically pick it up as an example or will it only be detected when --manifest-path is used?

[Nemo157]

Hmm, are you on Windows by any chance? It should be using a symlink for linking the examples in which is working for me on OS X. Checking AppVeyor it also doesn't appear to be building the examples as part of testcrate while Travis CI is... I'll try and come up with a solution that doesn't just involve copying the examples.

The problem with testing the examples as a part of uom is that cargo will pass all the feature flags to the examples as well. Having a separate testcrate isolates the tests from uom's feature flags, I've also noticed that uom's macros generate unit tests that fail when run outside uom which I also want to use the testcrate to test while fixing (I'll probably open an issue about these later this week).

[iliekturtles]

Yes, I'm on Windows. I noticed that tests/examples are given uom's features as well. I did a brief bit of searching and couldn't find any open issues in Cargo about it. I'm not sure if it should be considered an issue since there may be cases where you do want the features.

Perhaps the solution is moving to using a Cargo workspace and having multiple crates in the repository. A brief look shows that serde and diesel both seem to do this. If uom does end up needing custom derive (see #60) this may be the right route.

[Nemo157]

Checking the latest builds both AppVeyor and Travis CI built the examples as part of the testcrate.

I was just contemplating whether I should add testcrate into a workspace so that it doesn't need to rebuild all the dependencies for it. After seeing how long building uom twice on CI is (especially for AppVeyor) I'll push a commit doing this.

[iliekturtles]

testcrate/src/lib.rs doesn't seem to be necessary.

[iliekturtles]

Swap this line with the line below so that cargo test is run first when rustc isn't 1.20.0.

[iliekturtles]

Remove the duplicate blank line.

[iliekturtles]

Something doesn't seem to be correct with the workspace setup. https://doc.rust-lang.org/book/second-edition/ch14-03-cargo-workspaces.html says that running cargo test in the root should work for all crates within the workspace. That doesn't seem to be happening, but running cargo test -p testcrate does include those tests. If we can get that working then the appveyor and travis config files will only need a single cargo test line.

[Nemo157]

I believe this is because of one important distinction missing from that documentation, that is using a virtual manifest at the top-level of the workspace. In the next "Package Selection" section it's noted:

When default-members is not specified, the default is the root manifest if it is a package, or every member manifest (as if --all were specified on the command-line) for virtual workspaces.

I could do one of two things:

1. Add the testcrate to the default members so running cargo test will test it as well by default.
2. Switch the CI configuration to use cargo test --all to test both crates at once.

I'm leaning towards the first option so that contributors don't need to know that this is a workspace when making small changes, they can just run cargo test like normal and all tests will be run.

[iliekturtles]

I'd like to see distinct examples between the two crates. Currently if there is an example that fails to compile it will show up for both uom and testcrate.

[Nemo157]

Unfortunately I don't see a way to do that nicely.

My first thought was to remove the examples from uom itself and only build them as part of testcrate, but that would make it impossible to run cargo run --example mks from the root.

Not compiling the example as a part of testcrate would defeat the purpose of testcrate, to make sure that the examples are not depending on the un-intentional propagation of features from library to example.

[iliekturtles]

I didn't see the part about the virtual manifest but I like your first option so that cargo test does include other crates in the workspace.

In regards to the examples I think we should keep usage examples in the main crate and just use the nested crate to test macro / feature issues where a linking crate doesn't have access to uom's features.

I'll try to get to reviewing your latest commits tomorrow.

[iliekturtles]

I did a bit of experimentation today, but didn't get very far. One thing I did come across is https://github.com/laumann/compiletest-rs -- this may allow for tests that don't inherit uom's features without adding a full workspace. I'll investigate tomorrow or this weekend if you don't get to it first.

[iliekturtles]

compiletest-rs just released a new version that supports stable. Manishearth/compiletest-rs#107

I also believe we can write a private macro that generates the storage_types_{v} macros by passing in all unique identifiers:

macro_rules! storage_types_type { ... }

storage_types_type!(storage_type_f32, f32);
storage_types_type!(storage_type_f64, f64);
...

[Nemo157]

storage_types_type is definitely a good idea, I had considered it briefly but when I remembered that concat_idents doesn't work I gave up, but there's definitely enough code per-type to be worth macro-izing it even if you need to pass in the unique idents.

[iliekturtles]

I spent some time wrestling with compiletest_rs to get it to recognize errors. Below is an initial pass at a test. Still need to find a way to ensure that the uom is compiled with f32 and without i32 or to skip the test otherwise.

diff --git a/Cargo.toml b/Cargo.toml
index 3ed5350..c95fe69 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -27,6 +27,7 @@ typenum = "1.9"
 
 [dev-dependencies]
 approx = "0.1"
+compiletest_rs = { version = "0.3", features = ["stable"] }
 quickcheck = "0.6"
 serde_json = "1.0"
 static_assertions = "0.2"
diff --git a/tests/compile-fail/storage_types_feature_hygiene.rs b/tests/compile-fail/storage_types_feature_hygiene.rs
new file mode 100644
index 0000000..ec2595a
--- /dev/null
+++ b/tests/compile-fail/storage_types_feature_hygiene.rs
@@ -0,0 +1,13 @@
+#[macro_use]
+extern crate uom;
+
+storage_types! {
+    types: f32, i32;
+
+    pub fn do_work(v: V) {}
+}
+
+fn main() {
+    ::f32::do_work(0.0);
+    ::i32::do_work(0); //~ ERROR failed to resolve.
+}
diff --git a/tests/compiletest.rs b/tests/compiletest.rs
new file mode 100644
index 0000000..f0a45ef
--- /dev/null
+++ b/tests/compiletest.rs
@@ -0,0 +1,22 @@
+extern crate compiletest_rs as compiletest;
+
+use std::path::PathBuf;
+
+fn run_mode(mode: &'static str) {
+    let mut config = compiletest::Config::default();
+
+    config.mode = mode.parse().expect("Invalid mode");
+    config.src_base = PathBuf::from(format!("tests/{}", mode));
+
+    // config.link_deps(); // Populate config.target_rustcflags with dependencies on the path
+    config.target_rustcflags = Some("-L target/debug -L target/debug/deps".to_string());
+
+    config.clean_rmeta(); // If your tests import the parent crate, this helps with E0464
+
+    compiletest::run_tests(&config);
+}
+
+#[test]
+fn compile_fail() {
+    run_mode("compile-fail");
+}

[Nemo157]

Ok, pushed an update that removes the testcrate from this PR (I'll push a separate branch for that and open another PR to discuss just that vs compiletest vs any other potential solution).

I've added a storage_type_types macro to define all the macros, this required slightly changing the main macro to keep the attributes and token tree bundled up in parentheses so they can be treated as a single tt to workaround rust-lang/rust#35853. Now it's the @mod and @pub_mod branches that do the unbundling rather than the individual type branches.

[iliekturtles]

Could you apply the following patch and add something like "Part of #61." to the commit message and then I'll merge!

Formatting changes to storage_type_types! and sorts new macros in the same order as features are sorted elsewhere throughout the code:

 src/storage_types.rs | 48 ++++++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/src/storage_types.rs b/src/storage_types.rs
index d93d053..9ed8b0c 100644
--- a/src/storage_types.rs
+++ b/src/storage_types.rs
@@ -191,46 +191,42 @@ macro_rules! storage_types {
 
 macro_rules! storage_type_types {
     ($($macro_name:ident!($feature:tt, $name:ident, $($type:tt)+);)+) => {
-        $(
-            #[macro_export]
-            #[doc(hidden)]
-            #[cfg(feature = $feature)]
-            macro_rules! $macro_name {
-                ($attr:tt @$M:ident $tt:tt) => {
-                    storage_types!(@$M $attr $name, $($type)+; $tt);
-                };
-            }
+        $(#[macro_export]
+        #[doc(hidden)]
+        #[cfg(feature = $feature)]
+        macro_rules! $macro_name {
+            ($attr:tt @$M:ident $tt:tt) => {
+                storage_types!(@$M $attr $name, $($type)+; $tt);
+            };
+        }
 
-            #[macro_export]
-            #[doc(hidden)]
-            #[cfg(not(feature = $feature))]
-            macro_rules! $macro_name {
-                ($attr:tt @$M:ident $tt:tt) => {
-                };
-            }
-        )+
+        #[macro_export]
+        #[doc(hidden)]
+        #[cfg(not(feature = $feature))]
+        macro_rules! $macro_name {
+            ($attr:tt @$M:ident $tt:tt) => {
+            };
+        })+
     };
 }
 
 storage_type_types! {
+    storage_type_usize!("usize", usize, usize);
     storage_type_u8!("u8", u8, u8);
-    storage_type_i8!("i8", i8, i8);
     storage_type_u16!("u16", u16, u16);
-    storage_type_i16!("i16", i16, i16);
     storage_type_u32!("u32", u32, u32);
-    storage_type_i32!("i32", i32, i32);
-    storage_type_f32!("f32", f32, f32);
     storage_type_u64!("u64", u64, u64);
-    storage_type_i64!("i64", i64, i64);
-    storage_type_f64!("f64", f64, f64);
-    storage_type_usize!("usize", usize, usize);
     storage_type_isize!("isize", isize, isize);
-
+    storage_type_i8!("i8", i8, i8);
+    storage_type_i16!("i16", i16, i16);
+    storage_type_i32!("i32", i32, i32);
+    storage_type_i64!("i64", i64, i64);
     storage_type_bigint!("bigint", bigint, $crate::num::BigInt);
     storage_type_biguint!("biguint", biguint, $crate::num::BigUint);
-
     storage_type_rational!("rational", rational, $crate::num::Rational);
     storage_type_rational32!("rational32", rational32, $crate::num::rational::Rational32);
     storage_type_rational64!("rational64", rational64, $crate::num::rational::Rational64);
     storage_type_bigrational!("bigrational", bigrational, $crate::num::BigRational);
+    storage_type_f32!("f32", f32, f32);
+    storage_type_f64!("f64", f64, f64);
 }

[iliekturtles]

Thanks so much for the report about the issue and the PR to fix it! Took a while to get here but I appreciate all the effort you put in and think this solves the issue the right way. I'll try to get some tests completed tomorrow.