libstd: add an RAII utility for sys_common::mutex::Mutex #51529

nodakai · 2018-06-13T04:53:42Z

It is indeed debatable whether or not we should introduce more sophistication like this to the lowest layer of a system library. In fact, Drop::drop() cannot be unsafe (IIRC there was a discussion on introducing an unsafe variant of Drop whose entire scope must be within unsafe)

rust-highfive · 2018-06-13T04:53:46Z

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

oli-obk

What's the motivation for this change? I don't disapprove at all, I'm just curious!

oli-obk · 2018-06-13T05:19:40Z

src/libstd/io/lazy.rs

@@ -20,6 +20,9 @@ pub struct Lazy<T> {
    init: fn() -> Arc<T>,
 }

+// wish we had a "polymorphic constant" of type *mut Arc<T>...


You can create a generic const fn

oli-obk · 2018-06-13T05:22:15Z

src/libstd/sys/unix/args.rs

            let ret = (0..ARGC).map(|i| {
                let cstr = CStr::from_ptr(*ARGV.offset(i) as *const libc::c_char);
                OsStringExt::from_vec(cstr.to_bytes().to_vec())
            }).collect();
-            LOCK.unlock();
            return ret


The temporary ret can be removed now

oli-obk · 2018-06-13T05:22:43Z

src/libstd/sys/unix/os.rs

@@ -427,7 +426,6 @@ pub fn env() -> Env {
            iter: result.into_iter(),
            _dont_send_or_sync_me: PhantomData,
        };
-        ENV_LOCK.unlock();
        return ret


The temporary ret can be removed now

oli-obk · 2018-06-13T05:23:09Z

src/libstd/sys/unix/os.rs

        let s = libc::getenv(k.as_ptr()) as *const libc::c_char;
        let ret = if s.is_null() {
            None
        } else {
            Some(OsStringExt::from_vec(CStr::from_ptr(s).to_bytes().to_vec()))
        };
-        ENV_LOCK.unlock();
        return Ok(ret)


The temporary ret can be removed now

oli-obk · 2018-06-13T05:23:16Z

src/libstd/sys/unix/os.rs

        let ret = cvt(libc::setenv(k.as_ptr(), v.as_ptr(), 1)).map(|_| ());
-        ENV_LOCK.unlock();
        return ret


oli-obk · 2018-06-13T05:23:24Z

src/libstd/sys/unix/os.rs

@@ -480,9 +476,8 @@ pub fn unsetenv(n: &OsStr) -> io::Result<()> {
    let nbuf = CString::new(n.as_bytes())?;

    unsafe {
-        ENV_LOCK.lock();
+        let _guard = ENV_LOCK.lock();
        let ret = cvt(libc::unsetenv(nbuf.as_ptr())).map(|_| ());


oli-obk · 2018-06-13T05:25:16Z

src/libstd/sys_common/mutex.rs

+/// A simple RAII utility for the above Mutex without the poisoning semantics.
+pub struct MutexGuard<'a>(&'a imp::Mutex);
+
+impl<'a> Drop for MutexGuard<'a> {


I'm wondering whether a closure based approach would be better, since you are often creating a new scope for the lock guard anyway.

nodakai · 2018-06-14T02:27:49Z

@oli-obk Well, just a refactoring? sys_common/remutex.rs is another internal mutex library with an elaborated implementation of an RAII guard.

I wasn't sure about the flexibility of a closure-based approach (it surely works for the existing rustc codebase.) And we'd need unsafe { } around unsafe fn with_locked<T, F: FnOnce() -> T>(f: F) after all...

I will remove unnecessary returns

Signed-off-by: NODA, Kai <nodakai@gmail.com>

nodakai · 2018-06-17T16:42:14Z

@oli-obk Updated accordingly. Thanks for the comments!

oli-obk · 2018-06-17T18:31:55Z

@bors r+

bors · 2018-06-17T18:31:56Z

📌 Commit b81da27 has been approved by oli-obk

bors · 2018-06-17T20:11:41Z

⌛ Testing commit b81da27 with merge 86a8f1a...

libstd: add an RAII utility for sys_common::mutex::Mutex It is indeed debatable whether or not we should introduce more sophistication like this to the lowest layer of a system library. In fact, `Drop::drop()` cannot be `unsafe` (IIRC there was a discussion on introducing an unsafe variant of `Drop` whose entire scope must be within `unsafe`)

bors · 2018-06-17T22:25:29Z

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 86a8f1a to master...

tamird · 2018-06-22T18:00:46Z

src/libstd/sys/unix/os.rs

@@ -423,12 +422,10 @@ pub fn env() -> Env {
            }
            environ = environ.offset(1);
        }
-        let ret = Env {
+        return Env {


This return is unnecessary, right?

Unfortunately the inner function definition fn parse() below confuses the rustc parser if we omit the return

https://play.rust-lang.org/?gist=c39c88d10b00029672798633965295b3&version=stable&mode=debug

Oh, I see. This is pretty weird - I'd have expected the inner function to precede the actual code of the function (or to be inlined, since it has just one caller).

Thanks for explaining.

rust-highfive assigned withoutboats Jun 13, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2018

oli-obk requested changes Jun 13, 2018

View reviewed changes

nodakai force-pushed the improve-sys_common-mutex branch from 3859cf7 to 9ab19bc Compare June 17, 2018 06:58

libstd: add an RAII utility for sys_common::mutex::Mutex

b81da27

Signed-off-by: NODA, Kai <nodakai@gmail.com>

nodakai force-pushed the improve-sys_common-mutex branch from 9ab19bc to b81da27 Compare June 17, 2018 07:39

oli-obk approved these changes Jun 17, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2018

bors merged commit b81da27 into rust-lang:master Jun 17, 2018

nodakai deleted the improve-sys_common-mutex branch June 18, 2018 04:05

tamird reviewed Jun 22, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libstd: add an RAII utility for sys_common::mutex::Mutex #51529

libstd: add an RAII utility for sys_common::mutex::Mutex #51529

nodakai commented Jun 13, 2018

rust-highfive commented Jun 13, 2018

oli-obk left a comment

oli-obk Jun 13, 2018

oli-obk Jun 13, 2018

oli-obk Jun 13, 2018

oli-obk Jun 13, 2018

oli-obk Jun 13, 2018

oli-obk Jun 13, 2018

oli-obk Jun 13, 2018

nodakai commented Jun 14, 2018

nodakai commented Jun 17, 2018

oli-obk commented Jun 17, 2018

bors commented Jun 17, 2018

bors commented Jun 17, 2018

bors commented Jun 17, 2018

tamird Jun 22, 2018

nodakai Jun 22, 2018 •

edited

Loading

tamird Jun 22, 2018

libstd: add an RAII utility for sys_common::mutex::Mutex #51529

libstd: add an RAII utility for sys_common::mutex::Mutex #51529

Conversation

nodakai commented Jun 13, 2018

rust-highfive commented Jun 13, 2018

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodakai commented Jun 14, 2018

nodakai commented Jun 17, 2018

oli-obk commented Jun 17, 2018

bors commented Jun 17, 2018

bors commented Jun 17, 2018

bors commented Jun 17, 2018

Choose a reason for hiding this comment

nodakai Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodakai Jun 22, 2018 •

edited

Loading