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

fix: Make SortedList#add return value adhere to List contract #6049

Merged
merged 2 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main/java/spoon/support/util/SortedList.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@

@Override
public boolean addAll(Collection<? extends E> c) {
boolean ret = true;
boolean ret = false;
for (E e : c) {
ret &= add(e);
ret |= add(e);

Check warning on line 47 in src/main/java/spoon/support/util/SortedList.java

View workflow job for this annotation

GitHub Actions / code-quality qodana

Non-short-circuit boolean expression

Non-short-circuit boolean expression `ret |= add(e)`
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we'll always return true this way (because add always returns true), we can probably also just return !c.isEmpty() (assuming c doesn't get modified in between) or just do ret = true, similar to the default implementation in AbstractList. What do you think? (Seems like the Qodana inspection is a bit aggressive here...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Qodana is right i didn't notice it was using bit-wise operations instead of logical operations i have fixed that now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope the PR title is okay now

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your code before was fine, I definitely prefer it over the short-circuit solution now. I mainly suggested the alternatives because Qodana complained, but I still think the inspection is not helpful in such case at all.

I hope the PR title is okay now

Please give it a short description of what is addressed, so we can use it as commit message when merging (e.g. fix: address SortedList#addAll(...) contract violation). Also, please additionally prefix the title with review: given the PR is ready for review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i hope that the title is okay now... trying to follow the procedures that indicate that fic and the number of the issue needs to be present too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use one of my suggested variants or your initial version instead of the ||. The title is fine now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, i've reverted the change to use bit-wise logic again.

}
return ret;
}
Expand Down
154 changes: 154 additions & 0 deletions src/test/java/spoon/support/util/SortedListTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
package spoon.support.util;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;

import java.util.ArrayList;
import java.util.Comparator;
import java.util.LinkedList;
import java.util.List;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.function.Executable;

import spoon.reflect.declaration.CtClass;
import spoon.reflect.declaration.CtElement;

public class SortedListTest {
public class BasicComparator implements Comparator<CtElement> {

private final List<CtElement> myOrdering;

public BasicComparator(List<CtElement> orderList) {
this.myOrdering = orderList;
}

@Override
public int compare(CtElement o1, CtElement o2) {
int idxFirst = - 1;
int idxSecond = -1;
int idx = 1;
for(CtElement clazz : myOrdering) {
if (o1 == o2 && o1 == clazz) {
return 0;
} else if (clazz == o1) {
if (idxFirst == -1) {
idxFirst = idx;
idx++;
} else {
throw new IllegalStateException();
}

} else if (clazz == o2) {
if (idxSecond == -1) {
idxSecond = idx;
idx++;
} else {
throw new IllegalStateException();
}
};
}

return idxFirst - idxSecond;
}

}

@Test
public void sortedListUsesComparatorTestPass() {
Comparator<CtClass<?>> cmp1 = mock();
SortedList<CtClass<?>> sl1 = new SortedList<>(cmp1);
CtClass<?> lc1 = mock();
CtClass<?> lc2 = mock();
assertTrue(sl1.add(lc1), "Element should have been inserted");
assertTrue(sl1.add(lc2), "Element should have been inserted");
verify(cmp1).compare(lc2, lc1);
}

@Test
public void verifyAddWithIndexThrowsException() {
Comparator<CtClass<?>> cmp1 = mock();
SortedList<CtClass<?>> sl1 = new SortedList<>(cmp1);
CtClass<?> lc1 = mock();
IllegalArgumentException ex = assertThrows(
IllegalArgumentException.class,
new Executable() {

@Override
public void execute() throws Throwable {
sl1.add(1, lc1);
}

},
"Add with index should trhow IllegalArgumentException");
assertEquals(
"cannot force a position with a sorted list that has its own ordering",
ex.getMessage(),
"Exception message does not match the expected");
}

@Test
public void verifyThatGetComparatorReturnsTheComparatorTestPass() {
Comparator<CtClass<?>> cmp1 = mock();
SortedList<CtClass<?>> sl1 = new SortedList<>(cmp1);
assertTrue(cmp1 == sl1.getComparator(), "Comparator does not match the expected");
}

@Test
public void verifyThatComparatorCanBeReplacedTestPass() {
Comparator<? super CtClass<?>> cmp1 = mock();
SortedList<CtClass<?>> sl1 = new SortedList<>(cmp1);
BasicComparator bc1 = new BasicComparator(null);
sl1.setComparator(bc1);
assertTrue(bc1 == sl1.getComparator(), "Comparator does not match the expected");
}

@Test
public void addAllInsertsAllElementsAndTheyAreOrderedTestPass() {
List<CtElement> orderedList = new ArrayList<>(3);
CtClass<?> lc1 = mock();
CtClass<?> lc2 = mock();
CtClass<?> lc3 = mock();
orderedList.add(lc1);
orderedList.add(lc2);
orderedList.add(lc3);

BasicComparator bc1 = new BasicComparator(orderedList);
SortedList<CtElement> sl1 = new SortedList<>(bc1);

List<CtElement> unorderedList = new ArrayList<>(3);
unorderedList.add(lc3);
unorderedList.add(lc1);
unorderedList.add(lc2);
assertTrue(sl1.addAll(unorderedList), "All elements should have been added");

//Verify ordering
assertTrue(sl1.get(0) == lc1,
"First element does not match the expected ordering");
assertTrue(sl1.get(1) == lc2,
"Second element does not match the expected ordering");
assertTrue(sl1.get(2) == lc3,
"Third element does not match the expected ordering");
}

@Test
public void addAllWithEmptyListShouldLeaveTheListUnchangedTestPass() {
List<CtElement> orderedList = new ArrayList<>(3);
CtClass<?> lc1 = mock();
CtClass<?> lc2 = mock();
CtClass<?> lc3 = mock();
orderedList.add(lc1);
orderedList.add(lc2);
orderedList.add(lc3);

BasicComparator bc1 = new BasicComparator(orderedList);
SortedList<CtElement> sl1 = new SortedList<>(bc1);

assertTrue(sl1.add(lc1), "List should have changed");
assertFalse(sl1.addAll(new LinkedList<>()), "List should not have changed");
}
}
Loading