Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

fixup for #1602 - deprecate fixed cycle check#1717

Closed
MartinNowak wants to merge 1 commit intodlang:stablefrom
MartinNowak:fixup1602
Closed

fixup for #1602 - deprecate fixed cycle check#1717
MartinNowak wants to merge 1 commit intodlang:stablefrom
MartinNowak:fixup1602

Conversation

@MartinNowak
Copy link
Member

  • only print a deprecation warning for the fixed cycle detection
    cases (cycles over modules without ctor/dtor)

- only print a deprecation warning for the fixed cycle detection
  cases (cycles over modules without ctor/dtor)
@schveiguy
Copy link
Member

Looks close, but the issue is not that cycles with a non-ctor are a problem, the issue is with a cycle that goes through the same non-ctor module twice. The previous algorithm skipped that path.

So for instance with module a and c having ctors, and module b not, the following cycle was previously correctly detected:

a -> b -> c -> a

But this was not:

a -> b -> c -> b -> a.

I think that's the only thing to look for, a module cycle with 2 of the same non-ctor module in it. There could be more cases, but I think it's close enough to be acceptable.

If you want to add this extra detection, I will merge it.

@MartinNowak
Copy link
Member Author

MartinNowak commented Dec 19, 2016

That doesn't fix Higgs, I still get another fatal cycle after restricting the deprecation to the first one.

Deprecated: Cyclic dependency between module ir.inlining and ir.iir
ir.inlining* ->
ir.ast ->
ir.iir* ->
ir.ops ->
jit.ops* ->
ir.ast ->
ir.inlining*
object.Error@src/rt/minfo.d(346): Cyclic dependency between module jit.ops and ir.iir
jit.ops* ->
ir.ast ->
ir.iir* ->
ir.ops ->
jit.ops*

The problem was more generally that modules w/o ctors were immediately sorted in before all their deps were checked, so the module could already occur outside of the cycle.
a -> b* -> a -> c* -> d -> b*
Doesn't seem like we can detect this from the current implementation. Also running somewhat out of time on this.

@MartinNowak
Copy link
Member Author

An additional bitvector deprecatedCtorDone for non-relevant modules would work, but not that elegant.

@schveiguy
Copy link
Member

There also is the issue of the ordering of the modules. I believe if you reordered the modules, in some cases a cycle was then detected.

I think it's probably inelegant, but we may want to just run the old algorithm if we see any cycles, and act accordingly.

@MartinNowak
Copy link
Member Author

I think it's probably inelegant, but we may want to just run the old algorithm if we see any cycles, and act accordingly.

Yes, that was what I wanted to do, but then had this idea. Would you have a bit time for that?

@schveiguy
Copy link
Member

I will look into doing this later tonight if I can.

@schveiguy
Copy link
Member

OK, I think this should do it: #1720

@schveiguy schveiguy closed this Dec 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants