Skip to content

Conversation

vext01
Copy link
Contributor

@vext01 vext01 commented Apr 9, 2019

Hi,

Whilst fiddling around in the dominator code, I found some (I think) unused code. This code was used at the time it was imported, but over time it seems to have become redundant.

I've tested a build up to stage 1 with no problems. Maybe the tests will turn up something though.

P.S.

There is a FIXME comment in dominators/mod.rs:

    pub fn is_dominated_by(&self, node: Node, dom: Node) -> bool {                  
        // FIXME -- could be optimized by using post-order-rank                     
        self.dominators(node).any(|n| n == dom)                                     
    }

I'm not sure of the intention of this comment. The Dominators struct already operates over post-order rank nodes. Any ideas?

@rust-highfive
Copy link
Contributor

r? @nikomatsakis

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2019

@bors r+ rollup

Good catch. This wasn't removed when the uses were removed in de4dbe5#diff-8da257dba737a2318123538f324c54ceL162

@bors
Copy link
Collaborator

bors commented Apr 9, 2019

📌 Commit 3262d1e has been approved by oli-obk

@bors 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 Apr 9, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2019

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned nikomatsakis Apr 9, 2019
sanxiyn added a commit to sanxiyn/rust that referenced this pull request Apr 11, 2019
Kill dead code dominator code.

Hi,

Whilst fiddling around in the dominator code, I found some (I think) unused code. This code *was* used at the time it was imported, but over time it seems to have become redundant.

I've tested a build up to stage 1 with no problems. Maybe the tests will turn up something though.

P.S.

There is a FIXME comment in `dominators/mod.rs`:
```
    pub fn is_dominated_by(&self, node: Node, dom: Node) -> bool {
        // FIXME -- could be optimized by using post-order-rank
        self.dominators(node).any(|n| n == dom)
    }
```

I'm not sure of the intention of this comment. The `Dominators` struct already operates over post-order rank nodes. Any ideas?
@vext01
Copy link
Contributor Author

vext01 commented Apr 11, 2019

What about that FIXME?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 11, 2019

I'm not sure. I'm guessing it's outdated, but I haven't checked deeply

cramertj added a commit to cramertj/rust that referenced this pull request Apr 11, 2019
Kill dead code dominator code.

Hi,

Whilst fiddling around in the dominator code, I found some (I think) unused code. This code *was* used at the time it was imported, but over time it seems to have become redundant.

I've tested a build up to stage 1 with no problems. Maybe the tests will turn up something though.

P.S.

There is a FIXME comment in `dominators/mod.rs`:
```
    pub fn is_dominated_by(&self, node: Node, dom: Node) -> bool {
        // FIXME -- could be optimized by using post-order-rank
        self.dominators(node).any(|n| n == dom)
    }
```

I'm not sure of the intention of this comment. The `Dominators` struct already operates over post-order rank nodes. Any ideas?
Centril added a commit to Centril/rust that referenced this pull request Apr 12, 2019
Kill dead code dominator code.

Hi,

Whilst fiddling around in the dominator code, I found some (I think) unused code. This code *was* used at the time it was imported, but over time it seems to have become redundant.

I've tested a build up to stage 1 with no problems. Maybe the tests will turn up something though.

P.S.

There is a FIXME comment in `dominators/mod.rs`:
```
    pub fn is_dominated_by(&self, node: Node, dom: Node) -> bool {
        // FIXME -- could be optimized by using post-order-rank
        self.dominators(node).any(|n| n == dom)
    }
```

I'm not sure of the intention of this comment. The `Dominators` struct already operates over post-order rank nodes. Any ideas?
bors added a commit that referenced this pull request Apr 12, 2019
Rollup of 15 pull requests

Successful merges:

 - #59680 (Document the -Z flag to the rustc book)
 - #59711 (Add back the substring test)
 - #59806 (compiletest: Improve no_prefer_dynamic docs)
 - #59809 (Make trait_methods_not_found use a lock)
 - #59811 (Kill dead code dominator code.)
 - #59814 (Fix broken links on std::boxed doc page)
 - #59821 (improve unknown enum variant errors)
 - #59831 (Remove strange formatting in `Ordering` docs.)
 - #59836 (std::ops::Div examples: correct nominator to numerator)
 - #59857 (SGX target: fix cfg(test) build)
 - #59876 (Update TRPL to use mdbook 0.2)
 - #59880 (Remove note about transmute for float bitpatterns.)
 - #59889 (Update diagnostics.rs)
 - #59891 (Fix the link to sort_by_cached_key)
 - #59894 (save-analysis: Pull associated type definition using `qpath_def`)

Failed merges:

r? @ghost
@bors bors merged commit 3262d1e into rust-lang:master Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants