Initial review of parking_lot submodule

.travis.yml

@@ -7,6 +7,8 @@ rust:
 - beta
 - nightly
 
+# REVIEW: needs to be tested on OSX
+
 before_script:
 - |
   pip install 'travis-cargo<0.2' --user &&
@@ -39,6 +41,13 @@ script:
 - cargo run --release --bin rwlock -- 1 1 1 0 1 2
 - cd ..
 
+# REVIEW: needs a test that parking_lot still builds when structured like
+# libstd (aka including modules manually). This is easy to break by accident and
+# CI should help that import paths and such are always organized correctly.
+
+# REVIEW: needs at least one builder for all of `thread_parker` implementations
+# (at least building it, executing it would be best), missing ones are wasm/OSX
+
 env:
   global:
   - TRAVIS_CARGO_NIGHTLY_FEATURE=nightly

Cargo.toml

@@ -22,6 +22,7 @@ lazy_static = "1.0"
 bincode = {version = "1.1.3"}
 
 [build-dependencies]
+# REVIEW: would recommend `autocfg` instead of `rustc_version`
 rustc_version = "0.2"
 
 [features]

NOTES.md

@@ -0,0 +1,60 @@
+# Notes from review
+
+* WebKit's documentation and intro on parking lot and its paradigm is quite good
+
+* `cargo test` fails with compilation errors on Linux
+  * had an ancient lock file on my system and `cargo update` was needed
+
+* Various bits and pieces of the library are centered around compatibility with
+  various versions of Rust. This naturally isn't needed when integrating into
+  libstd, but it can often reduce the readability of code and/or make it more
+  difficult to follow. Do we want to have a clear version policy for parking_lot
+  on crates.io which is geared towards simplifying the code?
+
+* Overall quite worried about atomic orderings. Everything should be `SeqCst`
+  until proven otherwise, and even then needs quite a lot of proof (IMO) to not
+  use `SeqCst`. Being fast-and-loose with orderings seems like we're trying to
+  be too clever by half.
+
+* The `ThreadParker` API seems pretty subtle. For example the `UnparkHandle`
+  type can outlive the `ThreadParker` itself but that's ok. This seems currently
+  undocumented?
+
+* Can `parking_lot::Mutex` be used in as many places as the current OS-based
+  mutex? I think so but it's worth considering. For example you may be able to
+  use `std::sync::Mutex` today to protect a global allocator, but with parking
+  lot you won't be able to. Similarly about thread local internals with libstd,
+  but I think it's just internal stuff to libstd.
+
+* Where possible it'd be best to use `alloc`-the-crate now that it's stable
+  instead of using collections through `std`. Ideally only thread locals are
+  used from std (and other synchronization things if necessary).
+
+* There is a **massive** number of `#[inline]` annotations everywhere. I suspect
+  almost all of them are not necessary and can probably be removed. A
+  particularly egregious offender, for example, is `parking_lot_core::park`.
+  It's already generic and therefore inlined across crates, and it's a pretty
+  massive function to hint even more strongly that it should be inlined.
+
+* In general there's very little documentation beyond the bare bones required by
+  the `missing_docs` lint. Even that documentation isn't really anything beyond
+  what's already given in the function name and signature. Could benefit from
+  quite a few more comments about implementation strategies, protocols
+  implemented, meanings of constants, etc. More high level docs would also be
+  quite welcome explaining what each module is doing and how it relates to the
+  WTF parking lot.
+
+* It seems that `parking_lot_core` is fundamentally unsafe in a way that can't
+  really be library-ified? Given that `parking_lot`'s functions are `unsafe` due
+  to ensuring that the tokens aren't reused, it basically requires that
+  literally everything using the interface agrees on what tokens to allocate?
+  Using pointers works, but it seems like it's worthwhile documenting that it's
+  basically very difficult to use `parking_lot_core` in a safe fashion unless
+  you're doing the exact same thing as everyone else.
+
+* There's virtually no tests from what I can tell other than weak smoke tests
+  from the standard library. The `parking_lot_core` crate, for example, seems to
+  have virtually no tests of its APIs.
+
+* `Condvar` still can only be used with one mutex at a time (originally thought
+  this was going to be fixed)

appveyor.yml

@@ -27,3 +27,6 @@ test_script:
   - travis-cargo test
   - travis-cargo --only nightly test -- --features=deadlock_detection
   - travis-cargo doc
+
+# REVIEW: travis tests for linux seem more extensive, could configuration be
+# shared to test more on windows?

core/src/lib.rs

@@ -61,6 +61,13 @@
     feature(thread_local, checked_duration_since)
 )]
 
+// REVIEW: for maintainability this crate should unconditionally be
+// `#![no_std]`, and then the `libstd` module is exclusively used for importing
+// items from `std`. Items from `core` can be imported as usual.
+
+// REVIEW: the "i-am-libstd" feature would probably best be something like
+// `#[cfg(rustbuild)]` or something like that printed out by the build script in
+// rust-lang/rust.
 #[cfg(not(feature = "i-am-libstd"))]
 use cfg_if::cfg_if;
 
@@ -72,6 +79,11 @@ mod libstd {
 }
 
 cfg_if! {
+    // REVIEW: following the logic here of when and where `i-am-libstd` is
+    // included is somewhat confusing, it would probably be best to simply have
+    // libstd's own build script agree with parking_lot's own build script and
+    // print out the same `cfg` annotations that the `parking_lot` crate does on
+    // crates.io.
     if #[cfg(all(any(has_sized_atomics, feature = "i-am-libstd"), target_os = "linux"))] {
         #[path = "thread_parker/linux.rs"]
         mod thread_parker;
@@ -100,6 +112,9 @@ cfg_if! {
     } else if #[cfg(all(feature = "i-am-libstd", not(target_arch = "wasm32")))] {
         compile_error!("Not allowed to fall back to generic spin lock based thread parker");
     } else {
+        // REVIEW: this implementation isn't really appropriate for most use
+        // cases other than "at least it compiles", and for libstd we'll want a
+        // compile error if this is ever used.
         #[path = "thread_parker/generic.rs"]
         mod thread_parker;
     }