Allow features to be cyclical #4473
Conversation
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
| if f == feat { | ||
| bail!("Cyclic feature dependency: feature `{}` depends \ | ||
| on itself", feat); | ||
| } |
There was a problem hiding this comment.
This forbids direct dependency of feature on itself, right? Let's add a test case for this then?
There was a problem hiding this comment.
Yes that's correct. There was an existing test for this cyclic_feature() so I left it as-is.
| @@ -907,15 +907,22 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method) | |||
| } | |||
| None => { | |||
| let feat = feat_or_package; | |||
|
|
|||
| //if this feature has already been added, then just return Ok | |||
| if !visited.insert(feat) { | |||
There was a problem hiding this comment.
There's visited.remove(feat) down the line, and it seems suspicious to me.
Can it lead to visiting and walking the same feature more then once? Does anything break if we just remove the .remove? I would think it was needed for better error reporting only.
There was a problem hiding this comment.
I will look into that later today.
There was a problem hiding this comment.
I agree. It makes sense to remove it. I've pushed a fix for that.
wesleywiser
force-pushed
the
cyclical_features
branch
from
445217f to
3d966d3
Compare
|
Pinging @matklad in case they didn't see the updated commit. |
|
Thanks for the poke @wesleywiser ! @bors r+ |
|
📌 Commit 3d966d3 has been approved by |
|
No problem @matklad. Thanks for the review! |
Allow features to be cyclical Fixes #3796
|
☀️ Test successful - status-appveyor, status-travis |
Fixes #3796