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

Do not copy the list returned by findCandidates #554

Closed
wants to merge 1 commit into from

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Mar 9, 2024

The specification requires that we pass in the original list of candidates to the insertHostedCapability but currently always a copy is performed.

This replaces the copy by directly keeping the reference given as the resolver is woning the list anyways.

See

@laeubi laeubi requested a review from tjwatson March 9, 2024 06:10
Copy link

github-actions bot commented Mar 9, 2024

Test Results

   78 files  ±0     78 suites  ±0   37m 52s ⏱️ +50s
2 170 tests ±0  2 125 ✅ ±0   45 💤 ±0  0 ❌ ±0 
6 642 runs  ±0  6 505 ✅ ±0  137 💤 ±0  0 ❌ ±0 

Results for commit b9f603a. ± Comparison against base commit 49be75d.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Member Author

laeubi commented Mar 9, 2024

At least this seem not to break anything ...

The specification requires that we pass in the original list of
candidates to the insertHostedCapability but currently always a copy is
performed.

This replaces the copy by directly keeping the reference given as the
resolver is woning the list anyways.
@laeubi
Copy link
Member Author

laeubi commented Apr 21, 2024

@tjwatson what do you think?

@@ -48,8 +48,8 @@ private ShadowList(CandidateSelector shadow, List<Capability> original)
}

private ShadowList(List<Capability> unmodifiable, List<Capability> original, AtomicBoolean isUnmodifiable) {
super(unmodifiable, isUnmodifiable);
m_original = new ArrayList<Capability>(original);
super(new ArrayList<Capability>(unmodifiable), isUnmodifiable);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the switch here to copy for the super call and not for the m_original

Copy link
Member Author

Choose a reason for hiding this comment

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

This code a a bit cumbersome because it switches "unmodifiable" what actually is modified later on but the original is later passed to the ProvisonContext.

@laeubi
Copy link
Member Author

laeubi commented Apr 30, 2024

I'll close this for now as the specification will be changed in this aspect:

@laeubi laeubi closed this Apr 30, 2024
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