Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@paulirwin
paulirwin committed Feb 12, 2025
1 parent 7a3d35f commit 58524d3
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 23 deletions.
53 changes: 37 additions & 16 deletions src/Lucene.Net.Tests/Support/TestConcurrentHashSet.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ private static object[] LoadObjArray() // LUCENENET: avoid static constructors
// Ported from https://github.com/apache/harmony/blob/02970cb7227a335edd2c8457ebdde0195a735733/classlib/modules/luni/src/test/api/common/org/apache/harmony/luni/tests/java/util/CollectionsTest.java#L88-L159
public class SynchCollectionChecker : ThreadJob
{
private ISet<object> col; // LUCENENET: was Collection, but we need to use ISet to access the containsAll method
private ISet<object> col; // LUCENENET: was Collection, but we need to use ISet to access the IsSupersetOf method

// private int colSize; // LUCENENET: converted to local variable

Expand All @@ -95,8 +95,8 @@ public override void Run()
try
{
if (!(col.Count == 0
|| col.containsAll(normalCountingList)
|| col.containsAll(offsetCountingList)))
|| col.IsSupersetOf(normalCountingList)
|| col.IsSupersetOf(offsetCountingList)))
result = false;
col.clear();
}
Expand All @@ -105,9 +105,9 @@ public override void Run()
UninterruptableMonitor.Exit(col);
}
if (offset)
col.addAll(offsetCountingList);
col.UnionWith(offsetCountingList);
else
col.addAll(normalCountingList);
col.UnionWith(normalCountingList);
Interlocked.Increment(ref numberOfChecks); // was: numberOfChecks++;
}
}
Expand All @@ -128,9 +128,9 @@ public SynchCollectionChecker(ISet<object> c, bool offset,
offsetCountingList.Add(counter + colSize);
col.clear();
if (offset)
col.addAll(offsetCountingList);
col.UnionWith(offsetCountingList);
else
col.addAll(normalCountingList);
col.UnionWith(normalCountingList);
this.offset = offset; // LUCENENET - this line was missing from the original code
}

Expand Down Expand Up @@ -232,8 +232,8 @@ public void TestEquals()
public void TestContains()
{
ConcurrentHashSet<object> map = Map5();
assertTrue(map.contains(one));
assertFalse(map.contains(zero));
assertTrue(map.Contains(one));
assertFalse(map.Contains(zero));
}

/// <summary>
Expand Down Expand Up @@ -368,8 +368,30 @@ public void TestConstructor2()
}

// LUCENENET - testConstructor3 omitted, we don't have a single-int-argument constructor
// LUCENENET - omitted testGet_NullPointerException, testContainsKey_NullPointerException, testContainsValue_NullPointerException, etc.
// LUCENENET - omitted testPut_NullPointerException, etc. due to nullable reference type checking making them unnecessary
// LUCENENET - omitted *_NullPointerException tests that are not relevant

/// <summary>
/// Contains(null) should not throw.
/// </summary>
/// <remarks>
/// This differs from the ConcurrentHashMap tests in that we support null values.
/// </remarks>
[Test, LuceneNetSpecific]
public void TestNullSupport()
{
ConcurrentHashSet<object?> c = new ConcurrentHashSet<object?>
{
null // implicitly calls Add which should not throw on null
};
Assert.IsTrue(c.Contains(null));
c.Add(null); // should keep set the same
Assert.IsTrue(c.Contains(null));
Assert.AreEqual(1, c.Count);
Assert.IsTrue(c.TryRemove(null));
Assert.IsFalse(c.Contains(null));
Assert.AreEqual(0, c.Count);
}

// LUCENENET - omitted testSerialization due to lack of serialization support
// LUCENENET - omitted testSetValueWriteThrough as that is not applicable to a set

Expand Down Expand Up @@ -425,10 +447,9 @@ public void TestSynchronizedSet()
fail("join() interrupted");
}

ISet<object> mySet; // = new ConcurrentHashSet<object>(smallSet); // was: Collections.synchronizedSet(smallSet);
// LUCENENET: our type does not allow nulls
// mySet.add(null);
// assertTrue("Trying to use nulls in list failed", mySet.contains(null));
ISet<object?> mySet = new ConcurrentHashSet<object?>(smallSet); // was: Collections.synchronizedSet(smallSet);
mySet.Add(null);
assertTrue("Trying to use nulls in list failed", mySet.Contains(null));

smallSet = new HashSet<object>();
for (int i = 0; i < 100; i++)
Expand All @@ -440,7 +461,7 @@ public void TestSynchronizedSet()
// .RunTest();

//Test self reference
mySet = new ConcurrentHashSet<object>(smallSet); // was: Collections.synchronizedSet(smallSet);
mySet = new ConcurrentHashSet<object?>(smallSet); // was: Collections.synchronizedSet(smallSet);
mySet.add(mySet); // LUCENENET specific - references are not the same when wrapping via constructor, so adding mySet instead of smallSet
assertTrue("should contain self ref", Collections.ToString(mySet).IndexOf("(this", StringComparison.Ordinal) > -1);
}
Expand Down
17 changes: 10 additions & 7 deletions src/Lucene.Net/Support/ConcurrentHashSet.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Threading;
#nullable enable

Expand All @@ -44,7 +45,6 @@ namespace Lucene.Net.Support
/// </remarks>
[DebuggerDisplay("Count = {Count}")]
internal class ConcurrentHashSet<T> : ISet<T>, IReadOnlyCollection<T>
where T : notnull
{
private const int DefaultCapacity = 31;
private const int MaxLockNumber = 1024;
Expand Down Expand Up @@ -303,7 +303,7 @@ private ConcurrentHashSet(int concurrencyLevel, int capacity, bool growLockArray
/// <exception cref="T:OverflowException">The <see cref="ConcurrentHashSet{T}"/>
/// contains too many items.</exception>
public bool Add(T item)
=> AddInternal(item, _comparer.GetHashCode(item), true);
=> AddInternal(item, GetItemHashCode(item), true);

/// <summary>
/// Removes all items from the <see cref="ConcurrentHashSet{T}"/>.
Expand Down Expand Up @@ -338,7 +338,7 @@ private void ClearInternal()
/// <returns>true if the <see cref="ConcurrentHashSet{T}"/> contains the item; otherwise, false.</returns>
public bool Contains(T item)
{
var hashcode = _comparer.GetHashCode(item);
var hashcode = GetItemHashCode(item);

// We must capture the _buckets field in a local variable. It is set to a new table on each table resize.
var tables = _tables;
Expand Down Expand Up @@ -368,7 +368,7 @@ public bool Contains(T item)
/// <returns>true if an item was removed successfully; otherwise, false.</returns>
public bool TryRemove(T item)
{
var hashcode = _comparer.GetHashCode(item);
var hashcode = GetItemHashCode(item);
return TryRemoveInternal(item, hashcode, acquireLock: true);
}

Expand Down Expand Up @@ -495,7 +495,7 @@ private void InitializeFromCollection(IEnumerable<T> collection)
{
foreach (var item in collection)
{
AddInternal(item, _comparer.GetHashCode(item), false);
AddInternal(item, GetItemHashCode(item), false);
}

if (_budget == 0)
Expand Down Expand Up @@ -806,7 +806,7 @@ public void ExceptWith(IEnumerable<T> other)

foreach (var item in other)
{
TryRemoveInternal(item, _comparer.GetHashCode(item), acquireLock: false);
TryRemoveInternal(item, GetItemHashCode(item), acquireLock: false);
}
}
finally
Expand Down Expand Up @@ -887,14 +887,17 @@ public void UnionWith(IEnumerable<T> other)
AcquireAllLocks(ref locksAcquired);

foreach (var item in other)
AddInternal(item, _comparer.GetHashCode(item), acquireLock: false);
AddInternal(item, GetItemHashCode(item), acquireLock: false);
}
finally
{
ReleaseLocks(0, locksAcquired);
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private int GetItemHashCode(T item) => item is not null ? _comparer.GetHashCode(item) : 0;

private class Tables
{
public readonly Node?[] Buckets;
Expand Down

0 comments on commit 58524d3

Please sign in to comment.