GitHub PullRequestReviewCommentEvent at 2014-02-27 00:09:08
Repository: rust
Actor: huonw
Payload: {"comment":{"url":"https://api.github.com/repos/mozilla/rust/pulls/comments/10106467","id":10106467,"diff_hunk":"@@ -657,248 +655,283 @@ pub fn pat_is_ident(pat: @ast::Pat) -> bool {\n }\n \n // HYGIENE FUNCTIONS\n+pub mod mtwt {\n+    use ast::*;\n+    use ast;\n \n-/// Extend a syntax context with a given mark\n-pub fn new_mark(m:Mrk, tail:SyntaxContext) -> SyntaxContext {\n-    new_mark_internal(m,tail,get_sctable())\n-}\n+    use std::cell::RefCell;\n+    use std::local_data;\n+    use std::rc::Rc;\n \n-// Extend a syntax context with a given mark and table\n-// FIXME #8215 : currently pub to allow testing\n-pub fn new_mark_internal(m: Mrk, tail: SyntaxContext, table: &SCTable)\n-                         -> SyntaxContext {\n-    let key = (tail,m);\n-    // FIXME #5074 : can't use more natural style because we're missing\n-    // flow-sensitivity. Results in two lookups on a hash table hit.\n-    // also applies to new_rename_internal, below.\n-    // let try_lookup = table.mark_memo.find(&key);\n-    let mut mark_memo = table.mark_memo.borrow_mut();\n-    match mark_memo.get().contains_key(&key) {\n-        false => {\n-            let new_idx = {\n-                let mut table = table.table.borrow_mut();\n-                idx_push(table.get(), Mark(m,tail))\n-            };\n-            mark_memo.get().insert(key,new_idx);\n-            new_idx\n-        }\n-        true => {\n-            match mark_memo.get().find(&key) {\n-                None => fail!(\"internal error: key disappeared 2013042901\"),\n-                Some(idxptr) => {*idxptr}\n+    use collections::HashMap;\n+\n+    /// Extend a syntax context with a given mark\n+    pub fn new_mark(m:Mrk, tail:SyntaxContext, ns: IdentKind) -> SyntaxContext {\n+        get_sctable(ns).borrow().with(|table| new_mark_internal(m, tail, table))\n+    }\n+\n+    // Extend a syntax context with a given mark and table\n+    // FIXME #8215 : currently pub to allow testing\n+    pub fn new_mark_internal(m: Mrk, tail: SyntaxContext, table: &SCTable)\n+                             -> SyntaxContext {\n+        let key = (tail,m);\n+        // FIXME #5074 : can't use more natural style because we're missing\n+        // flow-sensitivity. Results in two lookups on a hash table hit.\n+        // also applies to new_rename_internal, below.\n+        // let try_lookup = table.mark_memo.find(&key);\n+        let mut mark_memo = table.mark_memo.borrow_mut();\n+        match mark_memo.get().contains_key(&key) {\n+            false => {\n+                let new_idx = {\n+                    let mut table = table.table.borrow_mut();\n+                    idx_push(table.get(), Mark(m,tail))\n+                };\n+                mark_memo.get().insert(key,new_idx);\n+                new_idx\n+            }\n+            true => {\n+                match mark_memo.get().find(&key) {\n+                    None => fail!(\"internal error: key disappeared 2013042901\"),\n+                    Some(idxptr) => {*idxptr}\n+                }\n             }\n         }\n     }\n-}\n-\n-/// Extend a syntax context with a given rename\n-pub fn new_rename(id:Ident, to:Name, tail:SyntaxContext) -> SyntaxContext {\n-    new_rename_internal(id, to, tail, get_sctable())\n-}\n \n-// Extend a syntax context with a given rename and sctable\n-// FIXME #8215 : currently pub to allow testing\n-pub fn new_rename_internal(id: Ident,\n-                           to: Name,\n-                           tail: SyntaxContext,\n-                           table: &SCTable)\n-                           -> SyntaxContext {\n-    let key = (tail,id,to);\n-    // FIXME #5074\n-    //let try_lookup = table.rename_memo.find(&key);\n-    let mut rename_memo = table.rename_memo.borrow_mut();\n-    match rename_memo.get().contains_key(&key) {\n-        false => {\n-            let new_idx = {\n-                let mut table = table.table.borrow_mut();\n-                idx_push(table.get(), Rename(id,to,tail))\n-            };\n-            rename_memo.get().insert(key,new_idx);\n-            new_idx\n-        }\n-        true => {\n-            match rename_memo.get().find(&key) {\n-                None => fail!(\"internal error: key disappeared 2013042902\"),\n-                Some(idxptr) => {*idxptr}\n+    /// Extend a syntax context with a given rename\n+    pub fn new_rename(id:Ident, to:Name,\n+                      tail:SyntaxContext,\n+                      ns: IdentKind) -> SyntaxContext {\n+        get_sctable(ns).borrow()\n+                       .with(|table| new_rename_internal(id, to, tail, table))\n+    }\n+\n+    // Extend a syntax context with a given rename and sctable\n+    // FIXME #8215 : currently pub to allow testing\n+    pub fn new_rename_internal(id: Ident,\n+                               to: Name,\n+                               tail: SyntaxContext,\n+                               table: &SCTable)\n+                               -> SyntaxContext {\n+        let key = (tail,id,to);\n+        // FIXME #5074\n+        //let try_lookup = table.rename_memo.find(&key);\n+        let mut rename_memo = table.rename_memo.borrow_mut();\n+        match rename_memo.get().contains_key(&key) {\n+            false => {\n+                let new_idx = {\n+                    let mut table = table.table.borrow_mut();\n+                    idx_push(table.get(), Rename(id,to,tail))\n+                };\n+                rename_memo.get().insert(key,new_idx);\n+                new_idx\n+            }\n+            true => {\n+                match rename_memo.get().find(&key) {\n+                    None => fail!(\"internal error: key disappeared 2013042902\"),\n+                    Some(idxptr) => {*idxptr}\n+                }\n             }\n         }\n     }\n-}\n \n-/// Make a fresh syntax context table with EmptyCtxt in slot zero\n-/// and IllegalCtxt in slot one.\n-// FIXME #8215 : currently pub to allow testing\n-pub fn new_sctable_internal() -> SCTable {\n-    SCTable {\n-        table: RefCell::new(~[EmptyCtxt,IllegalCtxt]),\n-        mark_memo: RefCell::new(HashMap::new()),\n-        rename_memo: RefCell::new(HashMap::new()),\n+    /// Make a fresh syntax context table with EmptyCtxt in slot zero\n+    /// and IllegalCtxt in slot one.\n+    // FIXME #8215 : currently pub to allow testing\n+    pub fn new_sctable_internal() -> SCTable {\n+        SCTable {\n+            table: RefCell::new(~[EmptyCtxt,IllegalCtxt]),\n+            mark_memo: RefCell::new(HashMap::new()),\n+            rename_memo: RefCell::new(HashMap::new()),\n+        }\n     }\n-}\n \n-// fetch the SCTable from TLS, create one if it doesn't yet exist.\n-pub fn get_sctable() -> @SCTable {\n-    local_data_key!(sctable_key: @@SCTable)\n-    match local_data::get(sctable_key, |k| k.map(|k| *k)) {\n-        None => {\n-            let new_table = @@new_sctable_internal();\n-            local_data::set(sctable_key,new_table);\n-            *new_table\n-        },\n-        Some(intr) => *intr\n-    }\n-}\n+    type SCTables = HashMap<IdentKind, Rc<RefCell<SCTable>>>;\n \n-/// print out an SCTable for debugging\n-pub fn display_sctable(table : &SCTable) {\n-    error!(\"SC table:\");\n-    let table = table.table.borrow();\n-    for (idx,val) in table.get().iter().enumerate() {\n-        error!(\"{:4u} : {:?}\",idx,val);\n+    // fetch the SCTable from TLS, create one if it doesn't yet exist.\n+    pub fn get_sctable(ns: IdentKind) -> Rc<RefCell<SCTable>> {\n+        local_data_key!(sctable_key: Rc<RefCell<SCTables>>)\n+        local_data::get_mut(sctable_key, |opt_ts| {\n+            let tables = match opt_ts {\n+                None => {\n+                    let ts = Rc::new(RefCell::new(HashMap::new()));\n+                    local_data::set(sctable_key, ts.clone());\n+                    ts\n+                }\n+                Some(ts) => ts.clone()\n+            };\n+            tables.borrow().with_mut(|ts|\n+                ts.find_or_insert_with(ns, |_| Rc::new(RefCell::new(new_sctable_internal())))","path":"src/libsyntax/ast_util.rs","position":208,"original_position":208,"commit_id":"4b72feb2a75f990fb1c9c625d74bb1e4618b951a","original_commit_id":"4b72feb2a75f990fb1c9c625d74bb1e4618b951a","user":{"login":"huonw","id":1203825,"avatar_url":"https://avatars.githubusercontent.com/u/1203825","gravatar_id":"365f3c2704e4744e237651f3c2a7c411","url":"https://api.github.com/users/huonw","html_url":"https://github.com/huonw","followers_url":"https://api.github.com/users/huonw/followers","following_url":"https://api.github.com/users/huonw/following{/other_user}","gists_url":"https://api.github.com/users/huonw/gists{/gist_id}","starred_url":"https://api.github.com/users/huonw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/huonw/subscriptions","organizations_url":"https://api.github.com/users/huonw/orgs","repos_url":"https://api.github.com/users/huonw/repos","events_url":"https://api.github.com/users/huonw/events{/privacy}","received_events_url":"https://api.github.com/users/huonw/received_events","type":"User","site_admin":false},"body":"I don't quite understand why we need a hashmap of two things.\r\n\r\n```rust\r\nstruct SCTables {\r\n     lifetime: Rc<RefCell<SCTable>>\r\n     plain: Rc<RefCell<SCTable>>\r\n}\r\n```\r\n\r\nshould work. (Stored as just `SCTables` in TLD.)\r\n\r\n(I imagine this is most of the performance problem: hashing 1 bits has a *lot* of overhead.)","created_at":"2014-02-27T00:09:08Z","updated_at":"2014-02-27T00:09:08Z","html_url":"https://github.com/mozilla/rust/pull/12563#discussion_r10106467","pull_request_url":"https://api.github.com/repos/mozilla/rust/pulls/12563","_links":{"self":{"href":"https://api.github.com/repos/mozilla/rust/pulls/comments/10106467"},"html":{"href":"https://github.com/mozilla/rust/pull/12563#discussion_r10106467"},"pull_request":{"href":"https://api.github.com/repos/mozilla/rust/pulls/12563"}}}}