Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly update symbol dependencies when updating a cached module #180

Closed
wants to merge 1 commit into from

Conversation

ryuukk
Copy link

@ryuukk ryuukk commented Jun 30, 2022

Solves this particulate case

module a;

import b;

b.data // if we edit the module C, then the modulecache doesn't properly propagate the changes to module c and its dependents because of the ownership check
module b;
import c;
StructFromC data;
module c;

struct StructFromC
{}

Copy link
Member

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally the complexity is increased by this as you linearly go through the range instead of having an optimized tree search to find equal parts.

The opSlice seems to be a difference that could affect how this loop works, but that sounds to me rather that either the API usage to call .parts here is wrong or that the iterator has some bug that should be fixed.

Also this is missing a test showing how it was broken before but now working.

Comment on lines +479 to +484
if (part.name == newPart.name && part.location == newPart.location)
{
has = true;
r = newPart;
break;
}
Copy link
Member

@WebFreak001 WebFreak001 Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part seems useless, the part == newPart above should call DSymbol.opEquals which is implemented by part.name == newPart.name, so this if should never actually match

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the part == newPart only check the pointer, not the value?

Copy link
Author

@ryuukk ryuukk Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as for the test, i have no clue what to do lol, if the current tests pass, i guess it should be fine

@ryuukk
Copy link
Author

ryuukk commented Sep 12, 2022

What's the status on this?

@WebFreak001
Copy link
Member

moved to dlang-community/DCD#682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants